[prev in list] [next in list] [prev in thread] [next in thread]
List: cfe-commits
Subject: Re: [PATCH] D10599: [OPENMP 4.0] Initial support for '#pragma omp declare simd' directive.
From: Richard Smith <richard () metafoo ! co ! uk>
Date: 2015-07-31 21:54:00
Message-ID: CAOfiQq=AgBnEo4xGPFa6rucLccCn7VtvVFaWRSKXu=wQem10sQ () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
See also this unanswered question on the OpenMP forums:
http://openmp.org/forum/viewtopic.php?f=12&t=1532
On Fri, Jul 31, 2015 at 2:07 PM, Richard Smith <richard@metafoo.co.uk>
wrote:
> rsmith added inline comments.
>
> ================
> Comment at: lib/Parse/ParseOpenMP.cpp:171-177
> @@ +170,9 @@
> +
> + // Append the current token at the end of the new token stream so
> that it
> + // doesn't get lost.
> + CachedPragmas.push_back(Tok);
> + // Push back tokens for pragma.
> + PP.EnterTokenStream(CachedPragmas.data(), CachedPragmas.size(),
> + /*DisableMacroExpansion=*/true,
> + /*OwnsTokens=*/false);
> + // Parse pragma itself.
> ----------------
> ABataev wrote:
> > rsmith wrote:
> > > Why are you doing this delayed parsing? You still seem to have no
> tests that require it.
> > We need it for future parsing of clauses associated with the 'declare
> simd' construct. Some of the clauses may have references to function
> arguments. That's why I'm using delayed parsing: at first we need to parse
> a function along with arguments and only then we'll parse all the pragmas
> along with their clauses and references to args.
> > Of course currently it is not used, because this patch does not
> introduce parsing of clauses. It will be added later.
> This code won't work for that; you've left the scope of the function
> parameters, so lookup for them will fail here. Generally-speaking, we don't
> like speculative / untested code to be committed, and would prefer to hold
> back on those changes until we can actually test them in some way.
>
> Here's what I suggest: remove the delayed parsing code from here for this
> commit (parse the pragma first, then parse the nested declaration, then act
> on the result), and bring that code back once you introduce parsing for a
> clause that needs it, if indeed that's the right approach for those
> clauses. (As I mentioned before, if all they do is to provide a list of
> identifiers naming parameters, you don't need delay parsing and can instead
> store them as identifiers and look them up in Sema after the fact. On the
> other hand, if this pragma allows an arbitrary expression referencing a
> parameter to appear before the declaration of that parameter, then we'll
> need something like delayed parsing.)
>
>
> http://reviews.llvm.org/D10599
>
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits@cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
[Attachment #5 (text/html)]
<div dir="ltr">See also this unanswered question on the OpenMP forums: <a \
href="https://urldefense.proofpoint.com/v2/url?u=http-3A__openmp.org_forum_viewtopic.p \
hp-3Ff-3D12-26t-3D1532&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZIS \
M_ASROnREeq0cCxk&m=XTR_gSIsTiurAvqsiadJWyM7c3vY6NvoFeOVxu5514Q&s=hEFYo3xNVPiujUXIksczy \
vbKPL6NV2pLGTtul3aknuo&e=">http://openmp.org/forum/viewtopic.php?f=12&t=1532</a></div><div \
class="gmail_extra"><br><div class="gmail_quote">On Fri, Jul 31, 2015 at 2:07 PM, \
Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" \
target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex">rsmith added inline comments.<br> <span class=""><br>
================<br>
Comment at: lib/Parse/ParseOpenMP.cpp:171-177<br>
@@ +170,9 @@<br>
+<br>
+ // Append the current token at the end of the new token stream so that it<br>
+ // doesn't get lost.<br>
+ CachedPragmas.push_back(Tok);<br>
+ // Push back tokens for pragma.<br>
+ PP.EnterTokenStream(CachedPragmas.data(), CachedPragmas.size(),<br>
+ /*DisableMacroExpansion=*/true,<br>
+ /*OwnsTokens=*/false);<br>
+ // Parse pragma itself.<br>
----------------<br>
</span><span class="">ABataev wrote:<br>
> rsmith wrote:<br>
> > Why are you doing this delayed parsing? You still seem to have no tests \
that require it.<br> > We need it for future parsing of clauses associated with \
the 'declare simd' construct. Some of the clauses may have references to \
function arguments. That's why I'm using delayed parsing: at first we need to \
parse a function along with arguments and only then we'll parse all the pragmas \
along with their clauses and references to args.<br> > Of course currently it is \
not used, because this patch does not introduce parsing of clauses. It will be added \
later.<br> </span>This code won't work for that; you've left the scope of the \
function parameters, so lookup for them will fail here. Generally-speaking, we \
don't like speculative / untested code to be committed, and would prefer to hold \
back on those changes until we can actually test them in some way.<br> <br>
Here's what I suggest: remove the delayed parsing code from here for this commit \
(parse the pragma first, then parse the nested declaration, then act on the result), \
and bring that code back once you introduce parsing for a clause that needs it, if \
indeed that's the right approach for those clauses. (As I mentioned before, if \
all they do is to provide a list of identifiers naming parameters, you don't need \
delay parsing and can instead store them as identifiers and look them up in Sema \
after the fact. On the other hand, if this pragma allows an arbitrary expression \
referencing a parameter to appear before the declaration of that parameter, then \
we'll need something like delayed parsing.)<br> <div class="HOEnZb"><div \
class="h5"><br> <br>
<a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D10599&d \
=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxk&m=XTR_g \
SIsTiurAvqsiadJWyM7c3vY6NvoFeOVxu5514Q&s=-rkVXCFTz8TkVzNn2JeMnTFw5_Pr1lMCYM6tEdrAUKI&e=" \
rel="noreferrer" target="_blank">http://reviews.llvm.org/D10599</a><br> <br>
<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" rel="noreferrer" \
target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br> \
</div></div></blockquote></div><br></div>
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic