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

[Michael Spencer via llvm-commits]

On Thu, Feb 9, 2017 at 3:50 PM, Davide Italiano <davide at freebsd.org> 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.
> >
> > - Michael Spencer
>
> The fact they didn't break (yet) doesn't make the test less brittle =)
> There are some questions pending on the review, I really think you
> should address them.


What question have I not addressed?

As for removing instcombine, it still leaves the dead loads which is
confusing. However just running DCE removes them. Is that fine with you?

- Michael Spencer
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170209/f2f7bc19/attachment.html>