[PATCH] D25969: [AliasSetTracker] Make AST smarter about intrinsics that don't actually affect memory.

[via llvm-commits]

On 2016-10-26 15:39, Daniel Berlin via llvm-commits wrote:
> On Wed, Oct 26, 2016 at 12:28 PM, Chad Rosier
> <mcrosier at codeaurora.org> wrote:
> 
>> mcrosier added a comment.
>> 
>> In https://reviews.llvm.org/D25969#580042 [1], @mcrosier wrote:
>> 
>>> In https://reviews.llvm.org/D25969#579967 [2], @nemanjai wrote:
>>> 
>>>> This caused a failure on all PPC machines I have access to:
>>>>  AddressSanitizer-powerpc64le-linux ::
>> TestCases/use-after-scope-loop-bug.cc
>>> 
>>> 
>>> Looking now..
>> 
>> I believe I've figured out the underlying problem.  In short, LICM
>> is relying on lifetime markers to prevent loads/stores from being
>> moved out of a particular scope.
> 
> Can you file more details on a bug somewhere? This seems pretty
> broken, but also, without your patch:
> 
> dannyb at dannyb-macbookpro3 [12:32:05]
> [~/sources/llvm-clean/lib/Transforms/Scalar] [mssa-batch-renamer-2]
> -> % grep -i lifetime LICM.cpp
> 

Right.  LICM doesn't explicitly check for the lifetime intrinsics.  
Rather, the lifetime intrinsics cause the AliasSetTracker to collapse 
sets, which indirectly changes the behavior of LICM with uses ASTs.

>> From the broken test case:
>> int main() {
>> 
>> // Variable goes in and out of scope.
>> for (int i = 0; i < 3; ++i) {
>> int x[3] = {i, i, i};
>> p = x + i;
>> }
>> return *p;  // BOOM
>> 
>> }
>> 
>> LICM will sink the stores to x[3] after the loop, which is obviously
>> incorrect.
> 
> I don't see why?

I assumed creating stores to the array after the lifetime of the array 
had ended was incorrect, but I guess that's not really true given the 
alloc does not live inside the loop.

> First, let's ignore the c-semantics, it's broken as-written in c,
> since the variable is dead whether you sink the stores or not.  (ASAN
> is a separate issue and should disable moving past lifetime markers or
> something)
> 
> Assuming p is a pointer, sinking the stores to before *p is just as
> fine as it is now, as long as you move the lifetime start and end
> markers to go with it (or drop them).

That sounds reasonable.

> 
> I presume the alloca does not occur in the loop, only the lifetime
> start and end.

Correct.

> In particular, you should be able to simply drop all lifetime start
> and end, and get the same result.
> 
> IE lifetime start and end do not serve as a dependence here, only
> metadata. If the only thing preventing LICM is the lifetime markers,
> that's very wrong since they are non-guaranteed metadata

Yes, I agree.  That was the intend of the patch, but I didn't foresee th

> 
>> I've reverted the change in r285227.  Sorry for the breakage.
> 
> You should be able to fix this simply by either
> 1. dropping the lifetime markers (non-ideal) of things you sink.
> or
> 2.  extending the lifetime markers appropriately.
> 
> The algorithm for that is that when you eliminate or sink things past
> them, you move the end to wherever the lowest point you sunk to is.
> You move starts to a block that post-dominates both the new end and
> the old end.

Honestly, this is something I saw in passing and I'm not sure I want to 
tackle this problem at th e moment.  However, I'll make sure to create a 
details PR in the morning.  Perhaps, I'll have some free time in the 
future.

Thanks for the feedback Danny.

  Chad

> 
>> Repository:
>> rL LLVM
>> 
>> https://reviews.llvm.org/D25969 [3]
> 
> 
> 
> Links:
> ------
> [1] https://reviews.llvm.org/D25969#580042
> [2] https://reviews.llvm.org/D25969#579967
> [3] https://reviews.llvm.org/D25969
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits