Re: [PATCH] #pragma vectorize

Sun, 27 Apr 2014 17:04:27 -0700

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.
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

Attachment: pragma_loop-svn.patch
Description: Binary data


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
 



_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits























					Reply via email to