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

[Daniel Berlin via llvm-commits]

On Wed, Oct 26, 2016 at 2:30 PM, <mcrosier at codeaurora.org> wrote:

> 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.


Right.


>
>
> 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.


Right, this is really a test of "does this get translated in a way that
ASAN detects it", precisely because it's legal to do the sinking "in llvm
ir" but "not in c"

>
>

>
>
>> 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.
>

SGTM.
If there's a PR, and someone discovers it improves performance, i'm sure
it'll be gotten to eventually :)
Here, the PR now describes what to do and how to fix it, which is better
than most of our PR's :)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161027/1afeddc1/attachment.html>