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

Davide Italiano via llvm-commits llvm-commits at lists.llvm.org
Thu Feb 9 16:14:53 PST 2017


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.


-- 
Davide

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


More information about the llvm-commits mailing list