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

[Davide Italiano via llvm-commits]

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?

-- 
Davide

"There are no solved problems; there are only problems that are more
or less solved" -- Henri Poincare