----- Original Message ----- > From: "Tyler Nowicki" <[email protected]> > To: "Alexey Bataev" <[email protected]> > Cc: "Hal J. Finkel" <[email protected]>, "Chandler Carruth" > <[email protected]>, "Nadav Rotem" <[email protected]>, > "llvm cfe" <[email protected]> > Sent: Sunday, April 27, 2014 7:00:22 PM > Subject: Re: [PATCH] #pragma vectorize > > > > > Hi, > > > Thanks to everyone for all their feedback. Please review the updated > patch. > > > I modified the implementation to use attributed statements on the > loop stmts to pass loop hints to the code generator. The benefit of > using this method is that c++11 loop attributes won’t take too much > more work to support, such as defining the syntax. Also attributed > statements are part of the AST, along with their data. > > > > The syntax remains unchanged as: > > > #pragma loop vectorize(enable/disable/value) > interleave(enable/disable/value) > > > In response to Alexey Bataev: > 1. Multiple-inheritance: Fixed using attributes to pass loop hints > instead. > 2. Support range-based loop: Done. > 3. Loop Attributes: In theory this should work, but its untested. I > couldn’t find any attributes that apply to loops or statements in > general. > 4. Serialization/deserialization: Should be supported with attributed > stmts. > 5. Expressions: Fixed. The value is now interpreted as a constant > integer expression. > 6. Contradictory directives: I don’t think this is a problem because > there are only really two options vectorize and interleave. > Contradictions could be easily checked if you think it would be a > problem.
Either we should adopt some 'last wins' rule, like we do for command-line options, or we should generate an error. I can't think of a use case for last-wins right now, so I recommend that we check for duplicate clauses and generate an error. Thoughts? -Hal > 7. ast-dump and ast-print: Should be supported on attributed stmts. > 8. Templates: Done and tests have been added. > > > In response to Hal Finkel: > - I made the changes you suggested. If I missed anything please let > me know. > - I think the documentation should be added in an separate commit. > I’ll get started on that next. > - I don’t know Doug or Richard, could you add them to the cc-list. > > > Looking forward to more feedback, > > > Tyler > > > > > > > > On Apr 22, 2014, at 11:25 PM, Alexey Bataev < [email protected] > > wrote: > > > > > Another one thing. > It seems to me that currently this implementation does not support > template instantiation. Need a test for it. > Best regards, > Alexey Bataev > ============= > Software Engineer > Intel Compiler Team > Intel Corp. 23.04.2014 7:43, Alexey Bataev пишет: > > > Hi everybody, my comments. > > 1. Agree with Chandler about multiple inheritance. I think it would > be better to create class LoopWithHintsStmt inherited from Stmt and > make it a base class for all loops. > 2. Are you going to support this pragma for range-based for loop from > C++11 (CXXForRangeStmt in AST)? > 3. + if ( Tok.is (tok::annot_pragma_loop_hint) || > + Tok.is (tok::kw_while) || Tok.is (tok::kw_do) || Tok.is > (tok::kw_for)) { > + StmtResult NextStmt = ParseStatement(); > + > + if (!NextStmt.isUsable()) > + return NextStmt; > + > + Stmt *Loop = NextStmt.get(); > + if (isa<AttributedStmt>(Loop)) > + Loop = cast<AttributedStmt>(Loop)->getSubStmt(); > > This code does not allow using of attributed statements because it > expects only 'for', 'do' or 'while' right after #pragma. > > 4. There is no serialization/deserialization support for LoopHints. > Also add a test for it to verify that it works properly. > 5. Is it ok that currently pragma allows using only of literal > constants in it clauses, but not constant expressions? > 6. What if I use several incompatible clauses for the same loop? For > example, how it is supposed to work if I have #pragma > vectorize(enable) vectorize(disable)? > 7. No support for -ast-dump and -ast-print options. > > I strongly recommend to ask Doug Gregor, Richard Smith to review this > code. > -- > Best regards, > Alexey Bataev > ============= > Software Engineer > Intel Compiler Team > Intel Corp. > From: Tyler Nowicki [ mailto:[email protected] ] > Sent: Wednesday, April 23, 2014 3:54 AM > To: Hal Finkel; Chandler Carruth > Cc: Bataev, Alexey; Nadav Rotem; llvm cfe > Subject: Re: [PATCH] #pragma vectorize > > Please review this updated patch. It includes the changes we > discussed. Thanks for all your input! > > Tyler > > > > On Apr 22, 2014, at 2:30 PM, Tyler Nowicki < [email protected] > > wrote: > > > > Hi Hal, > > Thanks for the reply. > > Maybe we're looking at this the wrong way... what about? > > pragma loop vectorize(width/enable/disable) > interleave(count/enable/disable) > > I like this more, especially because its clear it applies only to > loops. > > > > > > > enable/disable don’t add > anything that isn’t already part of pragma vectorize enable/disable, > and specifying `#pragma vectorize disable’ would disable > interleaving. > > But that's a bug. Are you sure that's what happens? > > I could be mistaken. This is what is in LoopVectorize at the top of > processLoop() > > if (Hints.Force == 0) { > DEBUG(dbgs() << "LV: Not vectorizing: #pragma vectorize disable.\n"); > return false; > } > > And the unrolling occurs later in processLoop(). I thought it was a > feature… but yea, lets fix it. > > > > > > > > > As for safety, how about #pragma vectorize aggressive? > > I don't like that; *that* sounds like a cost-model adjustment. The > user is asserting something about the property of the loop, and we > should try to capture that property. Although this may just be > confusing, "vectorizable" is what we mean. > > `nodependence’? > > Thanks, > > Tyler > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > > > > -- Hal Finkel Assistant Computational Scientist Leadership Computing Facility Argonne National Laboratory _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
