[LLVMdev] question about enabling cfl-aa and collecting a57 numbers
George Burgess IV
george.burgess.iv at gmail.com
Wed Feb 11 19:11:51 PST 2015
r228901 (bugfix+test) — the TODO was denoting that code under it is experimental; thanks for catching that. :)
(I need to start using branches more)
George
> On Feb 11, 2015, at 5:20 PM, Hal Finkel <hfinkel at anl.gov> wrote:
>
> ----- Original Message -----
>> From: "George Burgess IV" <george.burgess.iv at gmail.com <mailto:george.burgess.iv at gmail.com>>
>> To: "Daniel Berlin" <dberlin at dberlin.org <mailto:dberlin at dberlin.org>>
>> Cc: "Hal Finkel" <hfinkel at anl.gov <mailto:hfinkel at anl.gov>>, "Chandler Carruth" <chandlerc at google.com <mailto:chandlerc at google.com>>, "Jiangning Liu"
>> <Jiangning.Liu at arm.com <mailto:Jiangning.Liu at arm.com>>, "Yogesh Chavan" <cs13m1012 at iith.ac.in <mailto:cs13m1012 at iith.ac.in>>, "llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu> for LLVM"
>> <llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>>
>> Sent: Wednesday, February 11, 2015 4:07:49 PM
>> Subject: Re: [LLVMdev] question about enabling cfl-aa and collecting a57 numbers
>>
>>
>> @Hal - Attached fix+test. Thanks for catching that
>
> The bug fix and test case LGTM. Please commit those.
>
> Regarding the new function in the patch:
>
> +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();
> +}
>
> Please don't commit that without filling in some text after the TODO :-)
>
> -Hal
>
>> @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
>>
>>
>
> --
> 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/60c46764/attachment.html>
More information about the llvm-commits
mailing list