[LLVMdev] question about enabling cfl-aa and collecting a57 numbers

George Burgess IV george.burgess.iv at gmail.com
Wed Feb 11 14:07:49 PST 2015


@Hal - Attached fix+test. Thanks for catching that
@Danny - Thanks

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

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 *only occur if
LLVM is bootstrapped by a compiler using solely CFLAA*. So I'll try to seek
out a few cases where we're not giving the correct answer and figure out
why. :)

George

[1] - Failing tests:
    LLVM :: CodeGen/ARM/vector-spilling.ll
    LLVM :: CodeGen/R600/scalar_to_vector.ll
    LLVM :: CodeGen/R600/si-vector-hang.ll
    LLVM :: CodeGen/Thumb2/2009-12-01-LoopIVUsers.ll
    LLVM :: CodeGen/X86/avx-basic.ll
    LLVM :: CodeGen/X86/avx-shuffle.ll
    LLVM :: CodeGen/X86/avx-splat.ll
    LLVM :: CodeGen/X86/avx-trunc.ll
    LLVM :: CodeGen/X86/avx-unpack.ll
    LLVM :: CodeGen/X86/avx-vpermil.ll
    LLVM :: CodeGen/X86/avx2-conversions.ll
    LLVM :: CodeGen/X86/avx2-shuffle.ll
    LLVM :: CodeGen/X86/pointer-vector.ll
    LLVM :: CodeGen/X86/psubus.ll
    LLVM :: CodeGen/X86/sse3-avx-addsub.ll
    LLVM :: CodeGen/X86/vec_cast2.ll
    LLVM :: CodeGen/X86/vec_shuffle-27.ll


On Tue, Feb 10, 2015 at 4:27 PM, Daniel Berlin <dberlin at dberlin.org> wrote:

> Sorry for the delay, i'll review these in the next few days.
>
>
> On Tue Feb 10 2015 at 10:27:29 AM Hal Finkel <hfinkel at anl.gov> wrote:
>
>> [moving to llvm-commits for patch review]
>>
>> ----- Original Message -----
>> > From: "George Burgess IV" <george.burgess.iv at gmail.com>
>> > To: "Hal J. Finkel" <hfinkel at anl.gov>
>> > Cc: "Chandler Carruth" <chandlerc at google.com>, "Jiangning Liu" <
>> Jiangning.Liu at arm.com>, "LLVM Developers Mailing
>> > List" <llvmdev at cs.uiuc.edu>, "Daniel Berlin" <dberlin at dberlin.org>,
>> "Yogesh Chavan" <cs13m1012 at iith.ac.in>
>> > Sent: Monday, February 9, 2015 11:25:38 AM
>> > Subject: Re: [LLVMdev] question about enabling cfl-aa and collecting
>> a57 numbers
>> >
>> >
>> >
>> > As promised, attached are two diffs:
>> >
>> >
>> > cflaa-asm-bugfix.diff fixes that we crashed given InlineAsm [1], and
>> > has a minor style update (remove trailing whitespace from comments)
>>
>> Can you please attach a test case?
>>
>> > cflaa-inttoptr-fix.diff fixes how we treat inttoptr/ptrtoint
>> > structures. I ended up implementing this with StratifiedAttrs, and
>> > will speak with Danny about his concerns with this approach
>> > (hopefully) later this week. It seems to meet the goal of not
>> > unifying everything, so I don’t see it being problematic. [2]
>>
>> 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.
>>
>> Thanks again,
>> Hal
>>
>> >
>> >
>> > Patches should be applied in the order noted above.
>> >
>> >
>> > [1] - The bug was, more generally, when we’re given two Value*s that
>> > weren’t in any way related to a single function (e.g. two globals, a
>> > global + InlineAsm, …), we’d crash. Early in implementing CFLAA, I
>> > wanted crashes because those are easy to detect/trace, and I had
>> > minimal knowledge of what was getting passed in. Now that we have an
>> > impl that’s hopefully mostly working, a debug print will suffice in
>> > these cases.
>> >
>> >
>> > [2] - This implies that given %A and %B, where %A = inttoptr %Arg (or
>> > %Arg = ptrtoint %A), CFLAA *may* report NoAlias iff %B’s set has no
>> > StratifiedAttrs.
>> >
>> >
>> > Thanks again to Yogesh for the bug report,
>> > George
>> >
>> >
>> >
>> >
>> >
>> >
>> >
>> >
>> >
>> >
>> >
>> > On Feb 5, 2015, at 4:46 PM, George Burgess IV <
>> > george.burgess.iv at gmail.com > wrote:
>> >
>> >
>> > +Yogesh -- he found bugs (llvm_unreachable reached -- looking into
>> > why now) and said that he'd be interested in helping. Yay!
>> >
>> >
>> > Status update: Toy implementation of properly handling
>> > inttoptr/ptrtoint conversions (see: the fix for #2 on Danny's list)
>> > passes tests. I need to do a bit of clean up (and want to test it
>> > more thoroughly), and will hopefully have that patch out + a fix for
>> > the bug noted above by the end of the weekend
>> >
>> >
>> >
>> > George
>> >
>> >
>> > On Wed, Feb 4, 2015 at 12:43 AM, George Burgess IV <
>> > george.burgess.iv at gmail.com > wrote:
>> >
>> >
>> >
>> > Sounds good, I'll reword that comment. Also, the assert you mentioned
>> > turned out to be a bad assumption when combined with how I foresee
>> > us handling inttoptr/ptrtoint in the future, so I'll just replace it
>> > with slightly more robust code. :)
>> >
>> >
>> > Thanks for the feedback,
>> > George
>> >
>> >
>> >
>> >
>> > On Tue, Feb 3, 2015 at 11:30 PM, Hal Finkel < hfinkel at anl.gov >
>> > wrote:
>> >
>> >
>> > Hi George,
>> >
>> > +// Given an Instruction, this will add it to the graph, along with
>> > any
>> >
>> > +// Instructions that are potentially only available from said
>> > Instruction
>> >
>> > I think this comment is somewhat misleading. You can't really have
>> > orphaned instructions: instructions that have been inserted into a
>> > basic block must appear in its linked list of instructions that
>> > you'll visit when you iterate over all of them. You can have
>> > constantexprs, and I think that's what you're try to say.
>> >
>> > + assert(Edge.From == Inst.get() &&
>> >
>> > + "Expected ConstantExpr edge `From` to evaluate to the
>> > ConstantExpr");
>> >
>> > Indentation is odd here.
>> >
>> > For algorithmic considerations, I think that Danny is certainly the
>> > best person to review these.
>> >
>> > -Hal
>> >
>> > ----- Original Message -----
>> > > From: "George Burgess IV" < george.burgess.iv at gmail.com >
>> > > To: "Hal J. Finkel" < hfinkel at anl.gov >
>> > > Cc: "Chandler Carruth" < chandlerc at google.com >, "Jiangning Liu" <
>> > > Jiangning.Liu at arm.com >, "LLVM Developers Mailing
>> > > List" < llvmdev at cs.uiuc.edu >, "Daniel Berlin" <
>> > > dberlin at dberlin.org >
>> > > Sent: Friday, January 30, 2015 10:34:43 PM
>> > > Subject: Re: [LLVMdev] question about enabling cfl-aa and
>> > > collecting a57 numbers
>> > >
>> > >
>> >
>> >
>> > > So, I split it up into three patches:
>> > >
>> > >
>> > > - cflaa-danny-fixes.diff are (some of?) the fixes that Danny gave
>> > > us
>> > > earlier for tests + the minimal modifications you’d need to make in
>> > > CFLAA to make them pass tests.
>> > > - cflaa-minor-bugfixes.diff consists primarily of a bug fix for
>> > > Argument handling — we’d always report NoAlias when one of the
>> > > given
>> > > variables was an entirely unused argument
>> > > (We never added the appropriate Argument StratifiedAttr)
>> > > - cflaa-constexpr-fix.diff - The fix for the constexpr behavior
>> > > we’ve
>> > > been seeing
>> > >
>> > >
>> > > Patches are meant to be applied in the order listed.
>> > >
>> > >
>> > > Also, I just wanted to thank everyone again for your help so far —
>> > > it’s greatly appreciated. :)
>> > >
>> > >
>> > > George
>> > >
>> > >
>> > > On Jan 30, 2015, at 11:56 AM, Hal Finkel < hfinkel at anl.gov > wrote:
>> > >
>> > > ----- Original Message -----
>> > >
>> > >
>> > > From: "George Burgess IV" < george.burgess.iv at gmail.com >
>> > > To: "Hal Finkel" < hfinkel at anl.gov >
>> > > Cc: "Chandler Carruth" < chandlerc at google.com >, "Jiangning Liu" <
>> > > Jiangning.Liu at arm.com >, "LLVM Developers Mailing
>> > > List" < llvmdev at cs.uiuc.edu >, "Daniel Berlin" <
>> > > dberlin at dberlin.org
>> > > >
>> > > Sent: Friday, January 30, 2015 10:29:07 AM
>> > > Subject: Re: [LLVMdev] question about enabling cfl-aa and
>> > > collecting
>> > > a57 numbers
>> > >
>> > > I had thought that the case that Danny had looked at had a constant
>> > > GEP, and so this constant might alias with other global pointers.
>> > > How is that handled now?
>> > > That issue had to do with that we assumed that for all arguments of
>> > > a
>> > > given Instruction, each argument was either an Argument,
>> > > GlobalValue, or Inst in `for (auto& Bb : Inst.getBasicBlockList())
>> > > for (auto& Inst : Bb.getInstList())`. ConstantExprs didn't fit into
>> > > this instruction, because they aren't reached by said nested loop.
>> > >
>> > >
>> > > With this fix, if we detect that there's a relevant ConstantExpr,
>> > > we'll look into it as if it were a regular Instruction inside of
>> > > Bb.getInstList(), which causes us to correctly detect the globals,
>> > > etc.
>> > >
>> > > Sounds good.
>> > >
>> > > Thanks!
>> > >
>> > > -Hal
>> > >
>> > >
>> > >
>> > >
>> > >
>> > > (I included a test case specifically for this -- it's ugly, but we
>> > > have ~3 nested GEPs with a global at the innermost GEP. It produces
>> > > the appropriate output)
>> > >
>> > >
>> > > George
>> > >
>> > >
>> > > On Fri, Jan 30, 2015 at 10:56 AM, Hal Finkel < hfinkel at anl.gov >
>> > > wrote:
>> > >
>> > >
>> > > ----- Original Message -----
>> > >
>> > >
>> > > From: "George Burgess IV" < george.burgess.iv at gmail.com >
>> > > To: "Daniel Berlin" < dberlin at dberlin.org >
>> > > Cc: "Chandler Carruth" < chandlerc at google.com >, "Hal Finkel" <
>> > > hfinkel at anl.gov >, "Jiangning Liu"
>> > > < Jiangning.Liu at arm.com >, "LLVM Developers Mailing List" <
>> > > llvmdev at cs.uiuc.edu >
>> > > Sent: Friday, January 30, 2015 8:15:55 AM
>> > > Subject: Re: [LLVMdev] question about enabling cfl-aa and
>> > > collecting a57 numbers
>> > >
>> > >
>> > >
>> > > I'm not exactly thrilled about the size of this diff -- I'll
>> > > happily
>> > > break it up into more manageable bits later today, because some of
>> > > it is test fixes, another bit is a minor bug fix, etc.
>> > >
>> > > Yes, please break it into independent parts.
>> > >
>> > >
>> > >
>> > >
>> > >
>> > > Important bit (WRT ConstantExpr): moved the loop body from
>> > > buildGraphFrom into a new function. The body has a few tweaks to
>> > > call constexprToEdges on all ConstantExprs that we encounter.
>> > > constexprToEdges, naturally, interprets a ConstantExpr (and all
>> > > nested ConstantExprs) and places the results into a
>> > > SmallVector<Edge>.
>> > >
>> > >
>> > > I'm assuming this method of handling ConstantExprs isn't 100%
>> > > correct
>> > > because I was told that handling them correctly would be more
>> > > difficult than I think it is. I can't quite figure out why, so
>> > > examples of cases that break my code would be greatly appreciated.
>> > > :)
>> > >
>> > > I had thought that the case that Danny had looked at had a constant
>> > > GEP, and so this constant might alias with other global pointers.
>> > > How is that handled now?
>> > >
>> > > Thanks again,
>> > > Hal
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > > George
>> > >
>> > >
>> > > On Mon, Jan 26, 2015 at 2:43 PM, George Burgess IV <
>> > > george.burgess.iv at gmail.com > wrote:
>> > >
>> > >
>> > >
>> > >
>> > > Inline
>> > >
>> > >
>> > > George
>> > >
>> > >
>> > >
>> > >
>> > > On Jan 26, 2015, at 1:05 PM, Daniel Berlin < dberlin at dberlin.org >
>> > > wrote:
>> > >
>> > >
>> > > George, given that, can you just build constexpr handling (it's not
>> > > as easy as you think) as a separate funciton and have it use it in
>> > > the right places?
>> > > Will do. :)
>> > >
>> > >
>> > >
>> > >
>> > >
>> > > FWIW, my current list of CFLAA issues is:
>> > >
>> > > 1. Unknown values (results from ptrtoint, incoming pointers, etc)
>> > > are
>> > > not treated as unknown. These should be done through graph edge (so
>> > > that they can be one way, otherwise, you will unify everything :P)
>> > >
>> > >
>> > >
>> > >
>> > > 2. Constexpr handling
>> > >
>> > >
>> > >
>> > >
>> > > ^^^ These are correctness issues. I'm pretty sure there are a few
>> > > more but i haven't finished auditing
>> > > 3. In a number of places we treat non-pointers as memory-locations
>> > > and unify them with pointers. This introduces a lot of spurious
>> > > aliasing.
>> > > 4. More generally, we induce a lot of spurious aliasing through
>> > > things at different dereference levels. In these cases, one may to
>> > > the other, but, for example, if we have a foo***, and a foo* (and
>> > > neither pointers to unknown things or escapes), the only way for
>> > > foo
>> > > *** to alias foo* is if there is a graph path with two dereferences
>> > > between them.
>> > > We seem to get this wrong sometimes. Agreed on all four. Though
>> > > naturally it should be fixed, I’d like to see how much of an issue
>> > > #4 ends up being when we properly deal with #3.
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > > On Sun Jan 25 2015 at 6:44:07 PM Chandler Carruth <
>> > > chandlerc at google.com > wrote:
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> > > On Sun, Jan 25, 2015 at 6:37 PM, George Burgess IV <
>> > > george.burgess.iv at gmail.com > wrote:
>> > >
>> > >
>> > >
>> > >
>> > > Fixing that still gives a wrong result, i haven't started to
>> > > track
>> > > down what *else* is going on here.
>> > >
>> > >
>> > > Running with the attached diff + a modified buildGraphFrom to
>> > > handle
>> > > the constexpr GEPs, we seem to flag everything in test2.ll
>> > > (conservatively) correctly.
>> > >
>> > >
>> > > Is `store` the only place we can expect to see these constexpr
>> > > analogs, or is just about anywhere fair game?
>> > >
>> > >
>> > > Any Value can be a ConstantExpr, so all operands to instructions
>> > > are
>> > > fair game.
>> > >
>> > >
>> > >
>> > >
>> > >
>> > > --
>> > > Hal Finkel
>> > > Assistant Computational Scientist
>> > > Leadership Computing Facility
>> > > Argonne National Laboratory
>> > >
>> > >
>> > >
>> > > --
>> > > Hal Finkel
>> > > Assistant Computational Scientist
>> > > Leadership Computing Facility
>> > > Argonne National Laboratory
>> > >
>> >
>> > --
>> > Hal Finkel
>> > Assistant Computational Scientist
>> > Leadership Computing Facility
>> > Argonne National Laboratory
>> >
>> >
>> >
>> >
>>
>> --
>> Hal Finkel
>> Assistant Computational Scientist
>> Leadership Computing Facility
>> Argonne National Laboratory
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150211/42c3032e/attachment.html>
-------------- next part --------------
diff --git a/lib/Analysis/CFLAliasAnalysis.cpp b/lib/Analysis/CFLAliasAnalysis.cpp
index 14e3212..6eabc5d 100644
--- a/lib/Analysis/CFLAliasAnalysis.cpp
+++ b/lib/Analysis/CFLAliasAnalysis.cpp
@@ -887,6 +887,15 @@ static void buildGraphFrom(CFLAliasAnalysis &Analysis, Function *Fn,
   }
 }
 
+static bool canSkipAddingToSets(Value *V) {
+  if (isa<Constant>(V) && !isa<GlobalValue>(V) && !isa<ConstantExpr>(V))
+    return true;
+
+  // TODO:
+  auto *Ty = V->getType();
+  return Ty->isIntOrIntVectorTy();
+}
+
 static FunctionInfo buildSetsFrom(CFLAliasAnalysis &Analysis, Function *Fn) {
   NodeMapT Map;
   GraphT Graph;
@@ -917,7 +926,7 @@ static FunctionInfo buildSetsFrom(CFLAliasAnalysis &Analysis, Function *Fn) {
     while (!Worklist.empty()) {
       auto Node = Worklist.pop_back_val();
       auto *CurValue = findValueOrDie(Node);
-      if (isa<Constant>(CurValue) && !isa<GlobalValue>(CurValue))
+      if (canSkipAddingToSets(CurValue))
         continue;
 
       for (const auto &EdgeTuple : Graph.edgesFor(Node)) {
@@ -926,7 +935,7 @@ static FunctionInfo buildSetsFrom(CFLAliasAnalysis &Analysis, Function *Fn) {
         auto &OtherNode = std::get<1>(EdgeTuple);
         auto *OtherValue = findValueOrDie(OtherNode);
 
-        if (isa<Constant>(OtherValue) && !isa<GlobalValue>(OtherValue))
+        if (canSkipAddingToSets(OtherValue))
           continue;
 
         bool Added;
diff --git a/test/Analysis/CFLAliasAnalysis/asm-global-bugfix.ll b/test/Analysis/CFLAliasAnalysis/asm-global-bugfix.ll
new file mode 100644
index 0000000..5e38456
--- /dev/null
+++ b/test/Analysis/CFLAliasAnalysis/asm-global-bugfix.ll
@@ -0,0 +1,16 @@
+; Test case for a bug where we would crash when we were requested to report
+; whether two values that didn't belong to a function (i.e. two globals, etc)
+; aliased.
+
+; RUN: opt < %s -cfl-aa -aa-eval -print-may-aliases -disable-output 2>&1 | FileCheck %s
+
+ at G = private unnamed_addr constant [1 x i8] c"\00", align 1
+
+; CHECK: Function: test_no_crash
+; CHECK: 0 no alias responses
+define void @test_no_crash() #0 {
+entry:
+  call i8* asm "nop", "=r,r"(
+       i8* getelementptr inbounds ([1 x i8]* @G, i64 0, i64 0))
+  ret void
+}


More information about the llvm-commits mailing list