[PATCH] #pragma vectorize

[Hal Finkel]

----- Original Message -----
> From: "Tyler Nowicki" <tnowicki at apple.com>
> To: "Alexey Bataev" <a.bataev at hotmail.com>
> Cc: "Hal J. Finkel" <hfinkel at anl.gov>, "Chandler Carruth" <chandlerc at gmail.com>, "Nadav Rotem" <nrotem at apple.com>,
> "llvm cfe" <cfe-commits at cs.uiuc.edu>
> 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 < a.bataev at hotmail.com >
> 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:tnowicki at apple.com ]
> 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 < tnowicki at apple.com >
> 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
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> 
> 
> 
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory