[PATCH] D12310: Introducing llvm.invariant.group.barrier intrinsic & Global Opt handling

Tue Sep 8 16:07:47 PDT 2015

On 8 September 2015 at 16:01, Piotr Padlewski <prazek at google.com> wrote:

> Richard gave better test that could really break it:
>
> A a;
> B *B = new (&a) B;
> void f() {
>   b->f();
>   new (&a) A;
>   a.f();}
>
> Idea is, to make add some set<Value*>, that will store values got after
> invariant.group.barrier, and reject all stores that use values from this
> set.
>

That's about right -- if possible, instead of rejecting them you just need
to call @invariant.group.barrier, assuming all users are instructions.

If the user is not an instruction, say a ConstantExpr or a global variable
initializer (which must be a ConstantExpr) then you'll need to reject.

On Tue, Sep 8, 2015 at 1:58 PM, Piotr Padlewski <prazek at google.com> wrote:
>
>>
>> On Tue, Sep 8, 2015 at 1:42 PM, Nick Lewycky <nlewycky at google.com> wrote:
>>
>>>
>>>
>>> On 8 September 2015 at 12:41, Piotr Padlewski <prazek at google.com> wrote:
>>>
>>>>
>>>>
>>>> On Tue, Sep 8, 2015 at 11:32 AM, Nick Lewycky <nlewycky at google.com>
>>>> wrote:
>>>>
>>>>> On 8 September 2015 at 11:28, Piotr Padlewski <prazek at google.com>
>>>>> wrote:
>>>>>
>>>>>> Prazek marked an inline comment as done.
>>>>>>
>>>>>> ================
>>>>>> Comment at: lib/CodeGen/CodeGenPrepare.cpp:1414-1417
>>>>>> @@ -1413,2 +1413,6 @@
>>>>>>      }
>>>>>> +    case Intrinsic::invariant_group_barrier:
>>>>>> +      II->replaceAllUsesWith(II->getArgOperand(0));
>>>>>> +      II->eraseFromParent();
>>>>>> +      return true;
>>>>>>      }
>>>>>> ----------------
>>>>>> nlewycky wrote:
>>>>>> > Do we also discard the !invariant.group metadata on instructions?
>>>>>> >
>>>>>> > The backend has pointers to the Value*'s, it would be bad if they
>>>>>> used the !invariant.group markers but all the invariant.group.barrier calls
>>>>>> had been removed.
>>>>>> The question is, does it make any difference? What I understand, is
>>>>>> that CodeGenPrepare is done just before machine code generation, which
>>>>>> means that additional metadata in load/stores doesn't make any difference.
>>>>>>
>>>>>
>>>>> Today it does not. My point is that machine code generation does look
>>>>> at Value*. It does things like alias analysis on the IR. Leaving the
>>>>> !invariant.group there is a trap for a future developer.
>>>>>
>>>>
>>>> Ok, I will get rid of it, but it is not part of this review.
>>>> http://reviews.llvm.org/D12026
>>>>
>>>
>>> OK.
>>>
>>> ================
>>>>>> Comment at: lib/Transforms/IPO/GlobalOpt.cpp:2507-2508
>>>>>> @@ -2506,1 +2506,4 @@
>>>>>>            continue;
>>>>>> +        } else if (II->getIntrinsicID() ==
>>>>>> Intrinsic::invariant_group_barrier) {
>>>>>> +          setVal(II, getVal(II->getOperand(0)));
>>>>>> +          DEBUG(dbgs() << "Passing through invariant.group.barrier
>>>>>> intrinsic.\n");
>>>>>> ----------------
>>>>>> nlewycky wrote:
>>>>>> > Why is this safe? We have
>>>>>> >   %p2 = call @invariant.group.barrier(%p1)
>>>>>> > and we set that %p2 just plain *is* %p1. Then nothing happens until
>>>>>> evaluation completes and we commit the changes, which means replacing %p2
>>>>>> with %p1. At the time, why won't that operation miscompile?
>>>>>> >
>>>>>> > I can think of two possible reasons, one is that %p1 may be
>>>>>> constrained (like, it may be a ConstantExpr) and the other is that we may
>>>>>> know that there are no remaining loads of either %p1 or %p2 with
>>>>>> !invariant.group on them.
>>>>>> >
>>>>>> > In any event, please answer in the form of a comment in this code.
>>>>>> :)
>>>>>> What do you mean about miscompile?
>>>>>> I think this is safe because global-opt doesn't care about
>>>>>> !invariant.group metadata, which means that getting rid of
>>>>>> invariant.group.barrier will not break anything.
>>>>>>
>>>>>
>>>>> That's a good start, but why is it impossible to get a case where
>>>>> replacing the pointer is something globalopt does, then the pointer is used
>>>>> by a load in a function that globalopt didn't look at?
>>>>>
>>>> I am wating for test case as we disscuss
>>>>
>>>
>>> @s1 = unnamed_addr internal global i8*
>>> @s2 = unnamed_addr internal global i8*
>>>
>>> define void @__global_var_init() {
>>>   %p1 = ...
>>>    %p2 = call i8* @invariant.group.barrier(i8* %p1)
>>>   store i8* %p1, i8** @s1
>>>   store i8* %p2, i8** @s2
>>>   ret void
>>> }
>>>
>>> define void @runtime_function() {
>>>   %p1 = load i8*, i8** @s1
>>>   %p2 = load i8*, i8** @s2
>>>   %A = load i8* %p1, !invariant.group A
>>>   store ...
>>>   %B = load i8* %p2, !invariant.group A
>>>   ...
>>> }
>>>
>>> The order of optimization is:
>>>   1. globalopt turns %p2 into %p1 internally
>>>   2. it puts %p1 into @s1 and @s2
>>>   3. globalopt evaluation succeeds, it commits %p1 to both @s1 and @s2
>>>   4. there is no need for @s1 and @s2, they are unnamed_addr and always
>>> contain the same value, they are folded into one (@s2->RAUW(@s1))
>>>   5. therefore %p1 and %p2 are combined (%p2->RAUW(%p1))
>>>   6. now both %A and %B are loads with the same SSA variable and
>>> invariant.group, but they used to have an invariant.group.barrier between
>>> them
>>>
>>> Nick
>>>
>>> Ok, but in this case we load the same value so having and not having
>> invariant.group.barrier between them doesn't change anything. I could be
>> wrong if load of %A and %B would be different, but we can't get case like
>> this.
>> Second thing, is that I don't think we would be able to perform non local
>> optimizations - if __global_bar_init() would be a normal function, then
>> when we would ask about memory dependencie of %B or %A, we would get
>> clobber,
>> because we would run on the beginning of function. Am I right?
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150908/4ad33d97/attachment-0001.html>