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

[Nick Lewycky via llvm-commits]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150908/f175b007/attachment.html>