<div dir="ltr">@Hal - Attached fix+test. Thanks for catching that<div>@Danny - Thanks</div><div><br></div><div>@Any - Assuming that we don't need to go back and revisit the patches I submitted, the next thing on our to-do list seems to be removing primitives as much as possible from set construction. Using CFLAA with the supplied diffs, this can be as easy as modifying `canSkipAddingToSets`. </div><div><br></div><div>I say "can be" because when we try to skip the adding of integers, a fair number of the CodeGen tests fail [1]. These failures will <b>only occur if LLVM is bootstrapped by a compiler using solely CFLAA</b>. So I'll try to seek out a few cases where we're not giving the correct answer and figure out why. :)</div><div><br></div><div>George</div><div><br></div><div>[1] - Failing tests:</div><div><div>    LLVM :: CodeGen/ARM/vector-spilling.ll</div><div>    LLVM :: CodeGen/R600/scalar_to_vector.ll</div><div>    LLVM :: CodeGen/R600/si-vector-hang.ll</div><div>    LLVM :: CodeGen/Thumb2/2009-12-01-LoopIVUsers.ll</div><div>    LLVM :: CodeGen/X86/avx-basic.ll</div><div>    LLVM :: CodeGen/X86/avx-shuffle.ll</div><div>    LLVM :: CodeGen/X86/avx-splat.ll</div><div>    LLVM :: CodeGen/X86/avx-trunc.ll</div><div>    LLVM :: CodeGen/X86/avx-unpack.ll</div><div>    LLVM :: CodeGen/X86/avx-vpermil.ll</div><div>    LLVM :: CodeGen/X86/avx2-conversions.ll</div><div>    LLVM :: CodeGen/X86/avx2-shuffle.ll</div><div>    LLVM :: CodeGen/X86/pointer-vector.ll</div><div>    LLVM :: CodeGen/X86/psubus.ll</div><div>    LLVM :: CodeGen/X86/sse3-avx-addsub.ll</div><div>    LLVM :: CodeGen/X86/vec_cast2.ll</div><div>    LLVM :: CodeGen/X86/vec_shuffle-27.ll</div></div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Feb 10, 2015 at 4:27 PM, Daniel Berlin <span dir="ltr"><<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Sorry for the delay, i'll review these in the next few days.<br><br></div><div class="HOEnZb"><div class="h5"><br><div class="gmail_quote">On Tue Feb 10 2015 at 10:27:29 AM Hal Finkel <<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">[moving to llvm-commits for patch review]<br>
<br>
----- Original Message -----<br>
> From: "George Burgess IV" <<a href="mailto:george.burgess.iv@gmail.com" target="_blank">george.burgess.iv@gmail.com</a>><br>
> To: "Hal J. Finkel" <<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>><br>
> Cc: "Chandler Carruth" <<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>>, "Jiangning Liu" <<a href="mailto:Jiangning.Liu@arm.com" target="_blank">Jiangning.Liu@arm.com</a>>, "LLVM Developers Mailing<br>
> List" <<a href="mailto:llvmdev@cs.uiuc.edu" target="_blank">llvmdev@cs.uiuc.edu</a>>, "Daniel Berlin" <<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>>, "Yogesh Chavan" <<a href="mailto:cs13m1012@iith.ac.in" target="_blank">cs13m1012@iith.ac.in</a>><br>
> Sent: Monday, February 9, 2015 11:25:38 AM<br>
> Subject: Re: [LLVMdev] question about enabling cfl-aa and collecting a57 numbers<br>
><br>
><br>
><br>
> As promised, attached are two diffs:<br>
><br>
><br>
> cflaa-asm-bugfix.diff fixes that we crashed given InlineAsm [1], and<br>
> has a minor style update (remove trailing whitespace from comments)<br>
<br>
Can you please attach a test case?<br>
<br>
> cflaa-inttoptr-fix.diff fixes how we treat inttoptr/ptrtoint<br>
> structures. I ended up implementing this with StratifiedAttrs, and<br>
> will speak with Danny about his concerns with this approach<br>
> (hopefully) later this week. It seems to meet the goal of not<br>
> unifying everything, so I don’t see it being problematic. [2]<br>
<br>
Okay. I'd prefer that Danny's thoughts on this end up recorded somewhere, so either we can do this on-list, or someone should provide a summary.<br>
<br>
Thanks again,<br>
Hal<br>
<br>
><br>
><br>
> Patches should be applied in the order noted above.<br>
><br>
><br>
> [1] - The bug was, more generally, when we’re given two Value*s that<br>
> weren’t in any way related to a single function (e.g. two globals, a<br>
> global + InlineAsm, …), we’d crash. Early in implementing CFLAA, I<br>
> wanted crashes because those are easy to detect/trace, and I had<br>
> minimal knowledge of what was getting passed in. Now that we have an<br>
> impl that’s hopefully mostly working, a debug print will suffice in<br>
> these cases.<br>
><br>
><br>
> [2] - This implies that given %A and %B, where %A = inttoptr %Arg (or<br>
> %Arg = ptrtoint %A), CFLAA *may* report NoAlias iff %B’s set has no<br>
> StratifiedAttrs.<br>
><br>
><br>
> Thanks again to Yogesh for the bug report,<br>
> George<br>
><br>
><br>
><br>
><br>
><br>
><br>
><br>
><br>
><br>
><br>
><br>
> On Feb 5, 2015, at 4:46 PM, George Burgess IV <<br>
> <a href="mailto:george.burgess.iv@gmail.com" target="_blank">george.burgess.iv@gmail.com</a> > wrote:<br>
><br>
><br>
> +Yogesh -- he found bugs (llvm_unreachable reached -- looking into<br>
> why now) and said that he'd be interested in helping. Yay!<br>
><br>
><br>
> Status update: Toy implementation of properly handling<br>
> inttoptr/ptrtoint conversions (see: the fix for #2 on Danny's list)<br>
> passes tests. I need to do a bit of clean up (and want to test it<br>
> more thoroughly), and will hopefully have that patch out + a fix for<br>
> the bug noted above by the end of the weekend<br>
><br>
><br>
><br>
> George<br>
><br>
><br>
> On Wed, Feb 4, 2015 at 12:43 AM, George Burgess IV <<br>
> <a href="mailto:george.burgess.iv@gmail.com" target="_blank">george.burgess.iv@gmail.com</a> > wrote:<br>
><br>
><br>
><br>
> Sounds good, I'll reword that comment. Also, the assert you mentioned<br>
> turned out to be a bad assumption when combined with how I foresee<br>
> us handling inttoptr/ptrtoint in the future, so I'll just replace it<br>
> with slightly more robust code. :)<br>
><br>
><br>
> Thanks for the feedback,<br>
> George<br>
><br>
><br>
><br>
><br>
> On Tue, Feb 3, 2015 at 11:30 PM, Hal Finkel < <a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a> ><br>
> wrote:<br>
><br>
><br>
> Hi George,<br>
><br>
> +// Given an Instruction, this will add it to the graph, along with<br>
> any<br>
><br>
> +// Instructions that are potentially only available from said<br>
> Instruction<br>
><br>
> I think this comment is somewhat misleading. You can't really have<br>
> orphaned instructions: instructions that have been inserted into a<br>
> basic block must appear in its linked list of instructions that<br>
> you'll visit when you iterate over all of them. You can have<br>
> constantexprs, and I think that's what you're try to say.<br>
><br>
> + assert(Edge.From == Inst.get() &&<br>
><br>
> + "Expected ConstantExpr edge `From` to evaluate to the<br>
> ConstantExpr");<br>
><br>
> Indentation is odd here.<br>
><br>
> For algorithmic considerations, I think that Danny is certainly the<br>
> best person to review these.<br>
><br>
> -Hal<br>
><br>
> ----- Original Message -----<br>
> > From: "George Burgess IV" < <a href="mailto:george.burgess.iv@gmail.com" target="_blank">george.burgess.iv@gmail.com</a> ><br>
> > To: "Hal J. Finkel" < <a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a> ><br>
> > Cc: "Chandler Carruth" < <a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a> >, "Jiangning Liu" <<br>
> > <a href="mailto:Jiangning.Liu@arm.com" target="_blank">Jiangning.Liu@arm.com</a> >, "LLVM Developers Mailing<br>
> > List" < <a href="mailto:llvmdev@cs.uiuc.edu" target="_blank">llvmdev@cs.uiuc.edu</a> >, "Daniel Berlin" <<br>
> > <a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a> ><br>
> > Sent: Friday, January 30, 2015 10:34:43 PM<br>
> > Subject: Re: [LLVMdev] question about enabling cfl-aa and<br>
> > collecting a57 numbers<br>
> ><br>
> ><br>
><br>
><br>
> > So, I split it up into three patches:<br>
> ><br>
> ><br>
> > - cflaa-danny-fixes.diff are (some of?) the fixes that Danny gave<br>
> > us<br>
> > earlier for tests + the minimal modifications you’d need to make in<br>
> > CFLAA to make them pass tests.<br>
> > - cflaa-minor-bugfixes.diff consists primarily of a bug fix for<br>
> > Argument handling — we’d always report NoAlias when one of the<br>
> > given<br>
> > variables was an entirely unused argument<br>
> > (We never added the appropriate Argument StratifiedAttr)<br>
> > - cflaa-constexpr-fix.diff - The fix for the constexpr behavior<br>
> > we’ve<br>
> > been seeing<br>
> ><br>
> ><br>
> > Patches are meant to be applied in the order listed.<br>
> ><br>
> ><br>
> > Also, I just wanted to thank everyone again for your help so far —<br>
> > it’s greatly appreciated. :)<br>
> ><br>
> ><br>
> > George<br>
> ><br>
> ><br>
> > On Jan 30, 2015, at 11:56 AM, Hal Finkel < <a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a> > wrote:<br>
> ><br>
> > ----- Original Message -----<br>
> ><br>
> ><br>
> > From: "George Burgess IV" < <a href="mailto:george.burgess.iv@gmail.com" target="_blank">george.burgess.iv@gmail.com</a> ><br>
> > To: "Hal Finkel" < <a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a> ><br>
> > Cc: "Chandler Carruth" < <a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a> >, "Jiangning Liu" <<br>
> > <a href="mailto:Jiangning.Liu@arm.com" target="_blank">Jiangning.Liu@arm.com</a> >, "LLVM Developers Mailing<br>
> > List" < <a href="mailto:llvmdev@cs.uiuc.edu" target="_blank">llvmdev@cs.uiuc.edu</a> >, "Daniel Berlin" <<br>
> > <a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a><br>
> > ><br>
> > Sent: Friday, January 30, 2015 10:29:07 AM<br>
> > Subject: Re: [LLVMdev] question about enabling cfl-aa and<br>
> > collecting<br>
> > a57 numbers<br>
> ><br>
> > I had thought that the case that Danny had looked at had a constant<br>
> > GEP, and so this constant might alias with other global pointers.<br>
> > How is that handled now?<br>
> > That issue had to do with that we assumed that for all arguments of<br>
> > a<br>
> > given Instruction, each argument was either an Argument,<br>
> > GlobalValue, or Inst in `for (auto& Bb : Inst.getBasicBlockList())<br>
> > for (auto& Inst : Bb.getInstList())`. ConstantExprs didn't fit into<br>
> > this instruction, because they aren't reached by said nested loop.<br>
> ><br>
> ><br>
> > With this fix, if we detect that there's a relevant ConstantExpr,<br>
> > we'll look into it as if it were a regular Instruction inside of<br>
> > Bb.getInstList(), which causes us to correctly detect the globals,<br>
> > etc.<br>
> ><br>
> > Sounds good.<br>
> ><br>
> > Thanks!<br>
> ><br>
> > -Hal<br>
> ><br>
> ><br>
> ><br>
> ><br>
> ><br>
> > (I included a test case specifically for this -- it's ugly, but we<br>
> > have ~3 nested GEPs with a global at the innermost GEP. It produces<br>
> > the appropriate output)<br>
> ><br>
> ><br>
> > George<br>
> ><br>
> ><br>
> > On Fri, Jan 30, 2015 at 10:56 AM, Hal Finkel < <a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a> ><br>
> > wrote:<br>
> ><br>
> ><br>
> > ----- Original Message -----<br>
> ><br>
> ><br>
> > From: "George Burgess IV" < <a href="mailto:george.burgess.iv@gmail.com" target="_blank">george.burgess.iv@gmail.com</a> ><br>
> > To: "Daniel Berlin" < <a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a> ><br>
> > Cc: "Chandler Carruth" < <a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a> >, "Hal Finkel" <<br>
> > <a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a> >, "Jiangning Liu"<br>
> > < <a href="mailto:Jiangning.Liu@arm.com" target="_blank">Jiangning.Liu@arm.com</a> >, "LLVM Developers Mailing List" <<br>
> > <a href="mailto:llvmdev@cs.uiuc.edu" target="_blank">llvmdev@cs.uiuc.edu</a> ><br>
> > Sent: Friday, January 30, 2015 8:15:55 AM<br>
> > Subject: Re: [LLVMdev] question about enabling cfl-aa and<br>
> > collecting a57 numbers<br>
> ><br>
> ><br>
> ><br>
> > I'm not exactly thrilled about the size of this diff -- I'll<br>
> > happily<br>
> > break it up into more manageable bits later today, because some of<br>
> > it is test fixes, another bit is a minor bug fix, etc.<br>
> ><br>
> > Yes, please break it into independent parts.<br>
> ><br>
> ><br>
> ><br>
> ><br>
> ><br>
> > Important bit (WRT ConstantExpr): moved the loop body from<br>
> > buildGraphFrom into a new function. The body has a few tweaks to<br>
> > call constexprToEdges on all ConstantExprs that we encounter.<br>
> > constexprToEdges, naturally, interprets a ConstantExpr (and all<br>
> > nested ConstantExprs) and places the results into a<br>
> > SmallVector<Edge>.<br>
> ><br>
> ><br>
> > I'm assuming this method of handling ConstantExprs isn't 100%<br>
> > correct<br>
> > because I was told that handling them correctly would be more<br>
> > difficult than I think it is. I can't quite figure out why, so<br>
> > examples of cases that break my code would be greatly appreciated.<br>
> > :)<br>
> ><br>
> > I had thought that the case that Danny had looked at had a constant<br>
> > GEP, and so this constant might alias with other global pointers.<br>
> > How is that handled now?<br>
> ><br>
> > Thanks again,<br>
> > Hal<br>
> ><br>
> ><br>
> ><br>
> ><br>
> ><br>
> ><br>
> ><br>
> > George<br>
> ><br>
> ><br>
> > On Mon, Jan 26, 2015 at 2:43 PM, George Burgess IV <<br>
> > <a href="mailto:george.burgess.iv@gmail.com" target="_blank">george.burgess.iv@gmail.com</a> > wrote:<br>
> ><br>
> ><br>
> ><br>
> ><br>
> > Inline<br>
> ><br>
> ><br>
> > George<br>
> ><br>
> ><br>
> ><br>
> ><br>
> > On Jan 26, 2015, at 1:05 PM, Daniel Berlin < <a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a> ><br>
> > wrote:<br>
> ><br>
> ><br>
> > George, given that, can you just build constexpr handling (it's not<br>
> > as easy as you think) as a separate funciton and have it use it in<br>
> > the right places?<br>
> > Will do. :)<br>
> ><br>
> ><br>
> ><br>
> ><br>
> ><br>
> > FWIW, my current list of CFLAA issues is:<br>
> ><br>
> > 1. Unknown values (results from ptrtoint, incoming pointers, etc)<br>
> > are<br>
> > not treated as unknown. These should be done through graph edge (so<br>
> > that they can be one way, otherwise, you will unify everything :P)<br>
> ><br>
> ><br>
> ><br>
> ><br>
> > 2. Constexpr handling<br>
> ><br>
> ><br>
> ><br>
> ><br>
> > ^^^ These are correctness issues. I'm pretty sure there are a few<br>
> > more but i haven't finished auditing<br>
> > 3. In a number of places we treat non-pointers as memory-locations<br>
> > and unify them with pointers. This introduces a lot of spurious<br>
> > aliasing.<br>
> > 4. More generally, we induce a lot of spurious aliasing through<br>
> > things at different dereference levels. In these cases, one may to<br>
> > the other, but, for example, if we have a foo***, and a foo* (and<br>
> > neither pointers to unknown things or escapes), the only way for<br>
> > foo<br>
> > *** to alias foo* is if there is a graph path with two dereferences<br>
> > between them.<br>
> > We seem to get this wrong sometimes. Agreed on all four. Though<br>
> > naturally it should be fixed, I’d like to see how much of an issue<br>
> > #4 ends up being when we properly deal with #3.<br>
> ><br>
> ><br>
> ><br>
> ><br>
> ><br>
> ><br>
> ><br>
> > On Sun Jan 25 2015 at 6:44:07 PM Chandler Carruth <<br>
> > <a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a> > wrote:<br>
> ><br>
> ><br>
> ><br>
> ><br>
> ><br>
> ><br>
> > On Sun, Jan 25, 2015 at 6:37 PM, George Burgess IV <<br>
> > <a href="mailto:george.burgess.iv@gmail.com" target="_blank">george.burgess.iv@gmail.com</a> > wrote:<br>
> ><br>
> ><br>
> ><br>
> ><br>
> > Fixing that still gives a wrong result, i haven't started to<br>
> > track<br>
> > down what *else* is going on here.<br>
> ><br>
> ><br>
> > Running with the attached diff + a modified buildGraphFrom to<br>
> > handle<br>
> > the constexpr GEPs, we seem to flag everything in test2.ll<br>
> > (conservatively) correctly.<br>
> ><br>
> ><br>
> > Is `store` the only place we can expect to see these constexpr<br>
> > analogs, or is just about anywhere fair game?<br>
> ><br>
> ><br>
> > Any Value can be a ConstantExpr, so all operands to instructions<br>
> > are<br>
> > fair game.<br>
> ><br>
> ><br>
> ><br>
> ><br>
> ><br>
> > --<br>
> > Hal Finkel<br>
> > Assistant Computational Scientist<br>
> > Leadership Computing Facility<br>
> > Argonne National Laboratory<br>
> ><br>
> ><br>
> ><br>
> > --<br>
> > Hal Finkel<br>
> > Assistant Computational Scientist<br>
> > Leadership Computing Facility<br>
> > Argonne National Laboratory<br>
> ><br>
><br>
> --<br>
> Hal Finkel<br>
> Assistant Computational Scientist<br>
> Leadership Computing Facility<br>
> Argonne National Laboratory<br>
><br>
><br>
><br>
><br>
<br>
--<br>
Hal Finkel<br>
Assistant Computational Scientist<br>
Leadership Computing Facility<br>
Argonne National Laboratory<br>
</blockquote></div>
</div></div></blockquote></div><br></div>