[llvm] r294632 - [LoadCombine] Fix combining of loads which span an aliasing store.

[Philip Reames via llvm-commits]

On 02/09/2017 04:14 PM, Davide Italiano via llvm-commits wrote:
> On Thu, Feb 9, 2017 at 3:42 PM, Michael Spencer <bigcheesegs at gmail.com> wrote:
>> On Thu, Feb 9, 2017 at 2:11 PM, Davide Italiano <davide at freebsd.org> wrote:
>>> On Thu, Feb 9, 2017 at 2:09 PM, Michael Spencer <bigcheesegs at gmail.com>
>>> wrote:
>>>> On Thu, Feb 9, 2017 at 2:01 PM, Davide Italiano <davide at freebsd.org>
>>>> wrote:
>>>>> On Thu, Feb 9, 2017 at 1:46 PM, Michael J. Spencer via llvm-commits
>>>>> <llvm-commits at lists.llvm.org> wrote:
>>>>>> Author: mspencer
>>>>>> Date: Thu Feb  9 15:46:49 2017
>>>>>> New Revision: 294632
>>>>>>
>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=294632&view=rev
>>>>>> Log:
>>>>>> [LoadCombine] Fix combining of loads which span an aliasing store.
>>>>>>
>>>>>> Fixes PR31517
>>>>>>
>>>>>> Differential Revision: https://reviews.llvm.org/D28922
>>>>>>
>>>>>> Modified:
>>>>>>      llvm/trunk/lib/Transforms/Scalar/LoadCombine.cpp
>>>>>>      llvm/trunk/test/Transforms/LoadCombine/load-combine-aa.ll
>>>>>>
>>>>>> Modified: llvm/trunk/lib/Transforms/Scalar/LoadCombine.cpp
>>>>>> URL:
>>>>>>
>>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/LoadCombine.cpp?rev=294632&r1=294631&r2=294632&view=diff
>>>>>>
>>>>>>
>>>>>> ==============================================================================
>>>>>> --- llvm/trunk/lib/Transforms/Scalar/LoadCombine.cpp (original)
>>>>>> +++ llvm/trunk/lib/Transforms/Scalar/LoadCombine.cpp Thu Feb  9
>>>>>> 15:46:49
>>>>>> 2017
>>>>>> @@ -245,13 +245,17 @@ bool LoadCombine::runOnBasicBlock(BasicB
>>>>>>     bool Combined = false;
>>>>>>     unsigned Index = 0;
>>>>>>     for (auto &I : BB) {
>>>>>> -    if (I.mayThrow() || (I.mayWriteToMemory() &&
>>>>>> AST.containsUnknown(&I))) {
>>>>>> +    if (I.mayThrow() || AST.containsUnknown(&I)) {
>>>>>>         if (combineLoads(LoadMap))
>>>>>>           Combined = true;
>>>>>>         LoadMap.clear();
>>>>>>         AST.clear();
>>>>>>         continue;
>>>>>>       }
>>>>>> +    if (I.mayWriteToMemory()) {
>>>>>> +      AST.add(&I);
>>>>>> +      continue;
>>>>>> +    }
>>>>>>       LoadInst *LI = dyn_cast<LoadInst>(&I);
>>>>>>       if (!LI)
>>>>>>         continue;
>>>>>>
>>>>>> Modified: llvm/trunk/test/Transforms/LoadCombine/load-combine-aa.ll
>>>>>> URL:
>>>>>>
>>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/LoadCombine/load-combine-aa.ll?rev=294632&r1=294631&r2=294632&view=diff
>>>>>>
>>>>>>
>>>>>> ==============================================================================
>>>>>> --- llvm/trunk/test/Transforms/LoadCombine/load-combine-aa.ll
>>>>>> (original)
>>>>>> +++ llvm/trunk/test/Transforms/LoadCombine/load-combine-aa.ll Thu Feb
>>>>>> 9
>>>>>> 15:46:49 2017
>>>>>> @@ -37,3 +37,24 @@ define i64 @test2(i32* nocapture readonl
>>>>>>     ret i64 %add
>>>>>>   }
>>>>>>
>>>>>> +%rec11 = type { i16, i16, i16 }
>>>>>> + at str = global %rec11 { i16 1, i16 2, i16 3 }
>>>>>> +
>>>>>> +; PR31517 - Check that loads which span an aliasing store are not
>>>>>> combined.
>>>>>> +define i16 @test3() {
>>>>>> +; CHECK-LABEL: @test3
>>>>>> +
>>>>>> +; CHECK: load i16, i16*
>>>>>> +; CHECK: store i16
>>>>>> +; CHECK: ret i16
>>>>>> +
>>>>>> +  %_tmp9 = getelementptr %rec11, %rec11* @str, i16 0, i32 1
>>>>>> +  %_tmp10 = load i16, i16* %_tmp9
>>>>>> +  %_tmp12 = getelementptr %rec11, %rec11* @str, i16 0, i32 0
>>>>>> +  store i16 %_tmp10, i16* %_tmp12
>>>>>> +  %_tmp13 = getelementptr %rec11, %rec11* @str, i16 0, i32 0
>>>>>> +  %_tmp14 = load i16, i16* %_tmp13
>>>>>> +  %_tmp15 = icmp eq i16 %_tmp14, 3
>>>>>> +  %_tmp16 = select i1 %_tmp15, i16 1, i16 0
>>>>>> +  ret i16 %_tmp16
>>>>>> +}
>>>>>>
>>>>>>
>>>>> I think it's a good idea to change this tests to not rely on
>>>>> instcombine anymore.
>>>>
>>>> The IR gets pretty ugly if you do that and makes the test harder to
>>>> understand.
>>>>
>>> Hmm, but that also makes the test more fragile as we rely on
>>> instcombine doing some cleanups before loadcombine. I don't think we
>>> want that dependency. What do you think?
>>>
>> It's been in tree for 2 years with no changes due to changes in instcombine.
>> So I think it's fine.
> Please either fix the tests or revert this change.

JFYI, asking for a revert of a new change due to an existing problem 
with an existing test seems a bit extreme to me. Particularly given the 
author was responsive and discussion was ongoing.  Having said that, I 
agree that removing the instcombine dependence was a good outcome here 
as well.

Philip