[PATCH] #pragma vectorize

[Richard Smith]

On Thu, May 29, 2014 at 1:11 PM, Tyler Nowicki <tnowicki at apple.com> wrote:

> Ping^4
>
> I made a tiny change. I noticed while working on the non-type template
> parameter patch that I unintentionally overload Atrr::getKind() with
> LoopHintAttr::getKind(). I changed the second argument in LoopHintAttr to
> Arg so it will generate getArg() instead.
>
> I have a feeling my pings are going into Richard’s spam mail folder. Could
> someone who works with him let him know.
>

I'm really sorry for the delay; I'm *hugely* backlogged on email.

>From a high level, I think the syntax of this pragma could be improved.
Instead of accepting an integer literal (or eventually a template
argument), I would think this should accept an arbitrary integer constant
expression. It's not reasonable to require people to put their loops into a
template in order to specify a width that isn't an integer literal. Also,
having an argument that can either be an identifier or a pseudokeyword
('enable' or 'disable') seems like an unfortunate design choice. I'm OK
with you going ahead with the current design on the understanding that
we'll keep discussing it and may change it to something better.

Detailed review comments (mostly very minor):

--- include/clang/Basic/Attr.td (revision 209824)
+++ include/clang/Basic/Attr.td (working copy)
[...]
+  // Returns true if Args can co-exist a statement. Enable and Value are
+  // compatible. Disable is only compatible with itself.

Should probably be '///' (though doxygen doesn't seem to index our
generated headers yet). Also "can co-exist *on* a statement" (I assume?).

[...]
+  let Documentation = [Undocumented];

Please add documentation for this (doesn't have to be part of this patch,
but we should document this before we consider it "done").

--- lib/AST/StmtPrinter.cpp (revision 209824)
+++ lib/AST/StmtPrinter.cpp (working copy)
+    // FIXME: This hack will be removed when printPretty
+    // has been modified to print pretty pragmas

"to pretty-print pragmas."

+      OS << "#pragma loop ";
+      LHA->print(OS, Policy);

It seems a bit weird to print the "#pragma" part of the directive here, but
print the newline in the attribute's `print` function. I guess this will go
away soon, though, so never mind...

--- lib/CodeGen/CGStmt.cpp (revision 209824)
+++ lib/CodeGen/CGStmt.cpp (working copy)
[...]
+    if (Arg == LoopHintAttr::Enable) {
+      Name = llvm::MDString::get(Context, "llvm.vectorizer.enable");
+      Value = Builder.getTrue();

I find this surprising. Should these both really be semantically identical:

  #pragma clang loop interleave(enable)
  #pragma clang loop vectorize(enable)

That does not match your documentation.

[...]
+  // Add llvm.loop MDNode to CondBr.
+  llvm::MDNode *LoopID = llvm::MDNode::get(Context, Metadata);
+  LoopID->replaceOperandWith(0, LoopID); // First op points to itself.
+
+  CondBr->setMetadata("llvm.loop", LoopID);

You should skip this if Metadata.empty() (that is, if there are attributes,
but none of them are loop hints).

--- lib/CodeGen/CodeGenFunction.h (revision 209824)
+++ lib/CodeGen/CodeGenFunction.h (working copy)
[...]
+  void EmitWhileStmt(const WhileStmt &S,
+                     ArrayRef<const Attr *> Attrs = ArrayRef<const Attr
*>());
+  void EmitDoStmt(const DoStmt &S,
+                  ArrayRef<const Attr *> Attrs = ArrayRef<const Attr *>());
+  void EmitForStmt(const ForStmt &S,
+                   ArrayRef<const Attr *> Attrs = ArrayRef<const Attr
*>());
[...]
+  EmitCXXForRangeStmt(const CXXForRangeStmt &S,
+                      ArrayRef<const Attr *> Attrs = ArrayRef<const Attr
*>());

Please use "= None" instead.

--- lib/Parse/ParsePragma.cpp (revision 209824)
+++ lib/Parse/ParsePragma.cpp (working copy)
[...]
+  PP.AddPragmaHandler(LoopHintHandler.get());

This should be

  PP.AddPragmaHandler("clang", LoopHintHandler.get());

That is, your pragma should be in the "clang" pragma namespace.

[...]
+    PragmaLoopHintInfo *Info =
+        (PragmaLoopHintInfo *)PP.getPreprocessorAllocator().Allocate(
+            sizeof(PragmaLoopHintInfo),
llvm::alignOf<PragmaLoopHintInfo>());

Can you use just

  auto *Info = new (PP.getPreprocessorAllocator()) PragmaLoopHintInfo;

?

--- lib/Sema/SemaStmtAttr.cpp (revision 209824)
+++ lib/Sema/SemaStmtAttr.cpp (working copy)
[...]
+  if (St->getStmtClass() != Stmt::DoStmtClass &&
+      St->getStmtClass() != Stmt::ForStmtClass &&
+      St->getStmtClass() != Stmt::CXXForRangeStmtClass &&
+      St->getStmtClass() != Stmt::WhileStmtClass) {

Do you intentionally not handle ObjCForCollectionStmt here? (I suppose
they're very unlikely to be vectorizable?)

On May 24, 2014, at 4:29 PM, Mark Heffernan <meheff at google.com> wrote:
>
> On Fri, May 23, 2014 at 1:16 PM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
>
>> I'd be happy to get a start on reviewing those patches, if you're
>> ready for it. :-)
>>
>
> Great!  I should have something ready to send out mid-week.
>
> Thanks,
> Mark
>
>
>>
>> ~Aaron
>>
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140529/fdcb9856/attachment.html>