<div dir="ltr"><div class="gmail_extra"><div class="gmail_extra">Thanks, this looks like the right approach.</div><div class="gmail_extra"><br></div><div class="gmail_extra"><br></div><div class="gmail_extra">+def err_pragma_loop_invalid_expression : Error<</div>
<div class="gmail_extra">+ "invalid argument; expected a constant integer expression">;</div><div class="gmail_extra"><br></div><div class="gmail_extra">The appropriate term is "integer constant expression".</div>
<div class="gmail_extra"><br></div><div class="gmail_extra"><br></div><div class="gmail_extra"><div class="gmail_extra">+ if (ValueExpr) {</div><div class="gmail_extra">+ llvm::APSInt ValueAPS;</div><div class="gmail_extra">
+ bool IsICE = ValueExpr->isIntegerConstantExpr(ValueAPS, CGM.getContext());</div><div class="gmail_extra">+ (void)IsICE; // So release build do not warn about unused variables</div><div class="gmail_extra">+ assert(IsICE &&</div>
<div class="gmail_extra">+ "Loop hint argument is not an integer constant expression");</div><div class="gmail_extra"><br></div><div class="gmail_extra"><div class="gmail_extra">Use 'ValueExpr->EvaluateKnownConstInt' here.</div>
<br></div><div class="gmail_extra"><br></div><div class="gmail_extra">+ ValueInt = ValueAPS.getSExtValue();</div><div class="gmail_extra">+ }</div><div><br></div><div>What if the value doesn't fit in 64 bits? Asserting is not a friendly response. Since you're looking for a 32-bit int anyway, maybe saturate at 0xFFFFF'FFFF?</div>
<div><div><br></div><div><br></div><div> struct PragmaLoopHintInfo {</div><div> Token Loop;</div><div>- Token Value;</div></div><div> Token Option;<br></div><div>+ std::pair<Token *, size_t> Values;<br></div><div>
<br></div><div>Either use ArrayRef or split this out into two separate members with names that indicate that one is the size of the other.</div><div><br></div><div><br></div><div><div>+ if (!Info)<br></div></div><div><div>
+ return LoopHint(); // Uninitialized loop hints are ignored.</div><div></div></div><div><br></div><div>Why do we even create an annotation token in this case?</div><div><br></div><div><br></div><div><div>+ // Suppress diagnostic messages so we can report and invalid expression if</div>
<div>+ // ParseConstantExpression fails.</div></div><div><br></div><div>Typo 'and'. But I don't think this is a good idea. If ParseConstantExpression fails, you should just let it report its diagnostic and bail out. It gives better diagnostics than you do.</div>
<div><br></div><div><br></div><div>+ PP.EnterTokenStream(Args, ArgSize, false, false);<br></div><div><br></div><div>You'll need to add a terminator token here to stop us falling off the end of the tokens of the expression into whatever follows, in cases like</div>
<div><br></div><div> #pragma clang loop vectorize_width(1 +)</div><div> 1</div><div> for (...) {}</div><div><br></div><div>We usually add a tok::eof token after the saved tokens for this purpose. Please also add /*comments*/ explaining what the 'false's mean in this call.</div>
<div><br></div><div><br></div><div><div>+ QualType QT = R.get()->getType();</div><div>+ if (!QT->isIntegerType() || QT->isBooleanType() || QT->isCharType()) {</div><div>+ Diag(Args[0].getLocation(), diag::err_pragma_loop_invalid_expression);</div>
<div>+ return LoopHint(); // Uninitialized loop hints are ignored.</div><div>+ }</div></div><div><br></div><div>This should be in Sema, not here. (In particular, you need to allow a dependent type here and repeat this check after instantiation.)</div>
<div><br></div><div><br></div><div><div>+ Token *TokenArray = new Token[ValueList.size()];</div></div><div><br></div><div>Allocate this on the Preprocessor's block allocator, instead of leaking it.</div><div><br></div>
<div><br></div><div><div>- if (!Hint.LoopLoc || !Hint.OptionLoc || !Hint.ValueLoc)</div><div>+ if (!Hint.LoopLoc || !Hint.OptionLoc ||</div><div>+ !(Hint.EnabledLoc || Hint.ValueExpr))</div><div> continue;</div>
</div><div><br></div><div>Maybe create a LoopHint::isInvalid() and put this there? Do you really need to check the loop and option locations here?</div><div><br></div><div><br></div><div>+bool Sema::DiagnoseLoopHintValue(Expr *ValueExpr) {<br>
</div><div><br></div><div>Should be 'CheckLoopHintValue', since it doesn't always diagnose.</div><div><br></div><div><br></div><div>+ if (!ValueExpr->isIntegerConstantExpr(ValueAPS, getASTContext()) ||<br>
</div><div><br></div><div>Use VerifyIntegerConstantExpression here; it gives much better diagnostics.</div><div><br></div><div><br></div><div>+ // Only transform non-type template parameters.<br></div><div>+ if (!LH->getValue() || !LH->getValue()->isValueDependent())<br>
</div><div><br></div><div>This is not correct. You need to transform LH if the value is instantiation-dependent too (also the comment is wrong...). The way we usually deal with this is:</div><div><br></div><div>Expr *TransformedExpr = getDerived().TransformExpr(LH->getValue()).get();<br>
</div><div>if (TransformedExpr == LH->getValue().get())</div><div> return LH;</div><div><br></div><div><br></div><div><div>+ /// \brief Transform the given attribute.</div><div>+ ///</div><div>+ /// By default, this routine transforms a statement by delegating to the</div>
<div>+ /// appropriate TransformXXXAttr function to transform a specific kind</div><div>+ /// of attribute. Subclasses may override this function to transform</div><div>+ /// attributed statements using some other mechanism.</div>
<div>+ ///</div><div>+ /// \returns the transformed attribute</div><div>+ const Attr *TransformAttr(const Attr *S);</div></div><div><br></div><div>I don't like that this gives us two completely different ways to instantiate attributes (TreeTransform::transformAttrs / Sema::InstantiateAttrs and this new function). I'm OK with this as a stopgap measure if you're prepared to clean this duplication up afterwards, but this is not reasonable as a long-term situation.</div>
<div><br></div><div>Please add tests for the case where the value is type-dependent.</div><div><br></div><div>On Mon, Jun 30, 2014 at 3:07 PM, Hal Finkel <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span> wrote:<br>
</div></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Tyler,<br>
<br>
Thanks for working on this!<br>
<br>
First, this patch contains a few things that looks like unrelated cleanup (formatting changes and extra CHECK lines in the tests). Please separate these out (and commit them separately).<br>
<br>
Except, not the formatting part of this change (because it makes the lines too long):<br>
<br>
- /// State of the loop optimization specified by the spelling.<br>
<br>
let Args = [EnumArgument<"Option", "OptionType",<br>
<br>
- ["vectorize", "vectorize_width", "interleave", "interleave_count",<br>
<br>
- "unroll", "unroll_count"],<br>
<br>
- ["Vectorize", "VectorizeWidth", "Interleave", "InterleaveCount",<br>
<br>
- "Unroll", "UnrollCount"]>,<br>
<br>
- DefaultIntArgument<"Value", 1>];<br>
<br>
+ ["vectorize", "vectorize_width", "interleave", "interleave_count", "unroll", "unroll_count"],<br>
<br>
+ ["Vectorize", "VectorizeWidth", "Interleave", "InterleaveCount", "Unroll", "UnrollCount"]>,<br>
<br>
+ DefaultBoolArgument<"Enabled", 0>,<br>
<br>
+ ExprArgument<"Value", 0>];<br>
<br>
And, regarding errors like this:<br>
+/* expected-error {{invalid argument; expected a positive integer value}} */ #pragma clang loop interleave_count(10 / 5 - 2)<br>
<br>
I think it is important that we provide the evaluated integer value in the error message (it is obvious in this example, but if the numbers some from preprocessor macros and/or template parameters, it likely won't be obvious). Similarly, if we're given something of the wrong type (like a floating-point value), we should provide the name of the bad type.<br>
<br>
Thanks again,<br>
Hal<br>
<div class=""><div class="h5"><br>
----- Original Message -----<br>
> From: "Tyler Nowicki" <<a href="mailto:tnowicki@apple.com">tnowicki@apple.com</a>><br>
> To: "llvm cfe" <<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a>>, "Hal Finkel" <<a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a>>, "Aaron Ballman" <<a href="mailto:aaron.ballman@gmail.com">aaron.ballman@gmail.com</a>>,<br>
> "Richard Smith" <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>><br>
> Cc: "Nadav Rotem" <<a href="mailto:nrotem@apple.com">nrotem@apple.com</a>><br>
> Sent: Friday, June 27, 2014 4:11:17 PM<br>
> Subject: [PATCH] Support constant expressions, including non-type template parameters, in pragma loop hints<br>
><br>
> Hi Hal, Aaron, Richard,<br>
><br>
> This patch adds support for constant expressions, including non-type<br>
> template parameters, in pragma loop hints.<br>
><br>
> Support for constant expressions is done using<br>
> ParseConstantExpression() in the pragma parser, modifying the<br>
> LoopHintAttr to hold an Expr, and transforming the LoopHintAttr<br>
> expression during template instantiation.<br>
><br>
> Previously it was suggested that I also split the LoopHintAttr into<br>
> an Attr for enable/disable loop hints and an Attr for loop hints<br>
> with an expression. I gave it a try but it caused a lot of<br>
> duplication without much of a reduction in the complexity of loop<br>
> hint handling. I am willing to give it another look if you think it<br>
> would be better in the long run.<br>
><br>
> Tyler<br>
><br>
><br>
><br>
<br>
</div></div><span class=""><font color="#888888">--<br>
Hal Finkel<br>
Assistant Computational Scientist<br>
Leadership Computing Facility<br>
Argonne National Laboratory<br>
</font></span></blockquote></div><br></div></div>