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

Tue Sep 8 16:01:16 PDT 2015

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.

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/112c3b04/attachment.html>