<div dir="ltr"><div>On Thu, May 29, 2014 at 1:11 PM, Tyler Nowicki <span dir="ltr"><<a href="mailto:tnowicki@apple.com" target="_blank">tnowicki@apple.com</a>></span> wrote:<br></div><div class="gmail_extra"><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"><div style="word-wrap:break-word"><div>Ping^4</div><div><br>
</div><div>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.</div>
<div><br></div><div>I have a feeling my pings are going into Richard’s spam mail folder. Could someone who works with him let him know.</div></div></blockquote><div><br></div><div>I'm really sorry for the delay; I'm *hugely* backlogged on email.</div>
<div><br></div><div>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.</div>
<div><br></div><div>Detailed review comments (mostly very minor):</div><div><br></div><div><div>--- include/clang/Basic/Attr.td<span class="" style="white-space:pre"> </span>(revision 209824)</div><div>+++ include/clang/Basic/Attr.td<span class="" style="white-space:pre"> </span>(working copy)</div>
</div><div>[...]</div><div><div>+ // Returns true if Args can co-exist a statement. Enable and Value are</div><div>+ // compatible. Disable is only compatible with itself.</div><div><br></div><div>Should probably be '///' (though doxygen doesn't seem to index our generated headers yet). Also "can co-exist *on* a statement" (I assume?).</div>
<div><br></div><div>[...]</div><div>+ let Documentation = [Undocumented];<br></div><div><br></div><div>Please add documentation for this (doesn't have to be part of this patch, but we should document this before we consider it "done").</div>
<div><br></div><div><div>--- lib/AST/StmtPrinter.cpp<span class="" style="white-space:pre"> </span>(revision 209824)</div><div>+++ lib/AST/StmtPrinter.cpp<span class="" style="white-space:pre"> </span>(working copy)</div>
</div><div class="gmail_extra"><div class="gmail_extra">+ // FIXME: This hack will be removed when printPretty</div><div class="gmail_extra">+ // has been modified to print pretty pragmas</div><div class="gmail_extra">
<br></div><div class="gmail_extra">"to pretty-print pragmas."</div><div class="gmail_extra"><br></div><div class="gmail_extra"><div class="gmail_extra">+ OS << "#pragma loop ";</div><div class="gmail_extra">
+ LHA->print(OS, Policy);</div><div class="gmail_extra"><br></div><div class="gmail_extra">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...</div>
<div class="gmail_extra"><br></div><div class="gmail_extra"><div class="gmail_extra">--- lib/CodeGen/CGStmt.cpp<span class="" style="white-space:pre"> </span>(revision 209824)</div><div class="gmail_extra">+++ lib/CodeGen/CGStmt.cpp<span class="" style="white-space:pre"> </span>(working copy)</div>
<div class="gmail_extra">[...]</div><div class="gmail_extra"><div class="gmail_extra">+ if (Arg == LoopHintAttr::Enable) {</div><div class="gmail_extra">+ Name = llvm::MDString::get(Context, "llvm.vectorizer.enable");</div>
<div class="gmail_extra">+ Value = Builder.getTrue();</div><div><br></div></div><div class="gmail_extra"><div>I find this surprising. Should these both really be semantically identical:</div><div><br></div><div> #pragma clang loop interleave(enable)</div>
<div> #pragma clang loop vectorize(enable)<br></div><div><br></div><div>That does not match your documentation.</div><div><br></div></div><div class="gmail_extra">[...]</div><div class="gmail_extra"><div class="gmail_extra">
+ // Add llvm.loop MDNode to CondBr.</div><div class="gmail_extra">+ llvm::MDNode *LoopID = llvm::MDNode::get(Context, Metadata);</div><div class="gmail_extra">+ LoopID->replaceOperandWith(0, LoopID); // First op points to itself.</div>
<div class="gmail_extra">+</div><div class="gmail_extra">+ CondBr->setMetadata("llvm.loop", LoopID);</div><div><br></div><div>You should skip this if Metadata.empty() (that is, if there are attributes, but none of them are loop hints).</div>
<div><br></div><div><div>--- lib/CodeGen/CodeGenFunction.h<span class="" style="white-space:pre"> </span>(revision 209824)</div><div>+++ lib/CodeGen/CodeGenFunction.h<span class="" style="white-space:pre"> </span>(working copy)</div>
</div><div>[...]</div><div><div>+ void EmitWhileStmt(const WhileStmt &S,</div><div>+ ArrayRef<const Attr *> Attrs = ArrayRef<const Attr *>());</div><div>+ void EmitDoStmt(const DoStmt &S,</div>
<div>+ ArrayRef<const Attr *> Attrs = ArrayRef<const Attr *>());</div><div>+ void EmitForStmt(const ForStmt &S,</div><div>+ ArrayRef<const Attr *> Attrs = ArrayRef<const Attr *>());</div>
</div><div>[...]</div><div><div>+ EmitCXXForRangeStmt(const CXXForRangeStmt &S,</div><div>+ ArrayRef<const Attr *> Attrs = ArrayRef<const Attr *>());<br><br></div></div><div>Please use "= None" instead.</div>
<div><br></div><div><div>--- lib/Parse/ParsePragma.cpp<span class="" style="white-space:pre"> </span>(revision 209824)</div><div>+++ lib/Parse/ParsePragma.cpp<span class="" style="white-space:pre"> </span>(working copy)</div>
</div><div>[...]</div><div>+ PP.AddPragmaHandler(LoopHintHandler.get());<br></div><div><br></div><div>This should be </div><div><br></div><div> PP.AddPragmaHandler("clang", LoopHintHandler.get());<br></div><div>
<br></div><div>That is, your pragma should be in the "clang" pragma namespace.</div><div><br></div><div>[...]</div><div><div>+ PragmaLoopHintInfo *Info =</div><div>+ (PragmaLoopHintInfo *)PP.getPreprocessorAllocator().Allocate(</div>
<div>+ sizeof(PragmaLoopHintInfo), llvm::alignOf<PragmaLoopHintInfo>());</div></div><div><br></div><div>Can you use just</div><div><br></div><div> auto *Info = new (PP.getPreprocessorAllocator()) PragmaLoopHintInfo;</div>
<div><br></div><div>?</div><div><br></div><div><div>--- lib/Sema/SemaStmtAttr.cpp<span class="" style="white-space:pre"> </span>(revision 209824)</div><div>+++ lib/Sema/SemaStmtAttr.cpp<span class="" style="white-space:pre"> </span>(working copy)</div>
</div><div>[...]</div><div><div>+ if (St->getStmtClass() != Stmt::DoStmtClass &&</div><div>+ St->getStmtClass() != Stmt::ForStmtClass &&</div><div>+ St->getStmtClass() != Stmt::CXXForRangeStmtClass &&</div>
<div>+ St->getStmtClass() != Stmt::WhileStmtClass) {</div></div><div><br></div><div>Do you intentionally not handle ObjCForCollectionStmt here? (I suppose they're very unlikely to be vectorizable?)</div></div>
</div></div><div class="gmail_extra"><br></div></div></div><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">
<div style="word-wrap:break-word"><div><div>On May 24, 2014, at 4:29 PM, Mark Heffernan <<a href="mailto:meheff@google.com" target="_blank">meheff@google.com</a>> wrote:</div><br><blockquote type="cite"><div dir="ltr">
<div class="gmail_extra"><div class="gmail_quote">On Fri, May 23, 2014 at 1:16 PM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span> wrote:<br>
<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"><div>I'd be happy to get a start on reviewing those patches, if you're<br>
</div>
ready for it. :-)<br></blockquote><div><br></div><div>Great! I should have something ready to send out mid-week.</div><div><br></div><div>Thanks,</div><div>Mark</div><div> </div><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">
<span><font color="#888888"><br>
~Aaron<br>
</font></span></blockquote></div><br></div></div>
</blockquote></div><br></div>
<br></blockquote></div><br></div></div>