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

[Bigcheese]

Summary

Fixes PR31517

I'm not 100% sure about this fix. The problem is we don't check if the last load aliases anything, so this now checks all instructions to see if they alias and adds stores to the alias set. However I'm not sure if memcpy/memset or any other ways of writing need to be handled specially here.

[efriedma]

See also https://reviews.llvm.org/D21496.

Special-casing store instructions doesn't make any sense; any instruction which writes to memory has the same properties here.

[davide]

Yes, I think you should revamp D21496.

[Bigcheese]

Handle any instruction which may write to memory.

[uabelho]

I don't know why but this patch seems to also make https://llvm.org/bugs/show_bug.cgi?id=31493 stop crashing.

[davide]

In D28922#653006, @uabelho wrote:

I don't know why but this patch seems to also make https://llvm.org/bugs/show_bug.cgi?id=31493 stop crashing.

Can you please also try with the code in D21496 ? IIRC it was ready modulo tests.

[uabelho]

In D28922#653261, @davide wrote:

In D28922#653006, @uabelho wrote:

I don't know why but this patch seems to also make https://llvm.org/bugs/show_bug.cgi?id=31493 stop crashing.

Can you please also try with the code in D21496 ? IIRC it was ready modulo tests.

Ok, PR31493 also stops crashing if applying only D21496.

[davide]

This is definitely a stop-gap solution to the problem, and although we seem to have a more general patch already available, you don't have time/don't want to work on it.
As this fixes a miscompile and a crash reported upstream, I'm inclined to see this patch or a modified form of it upstream. River can land his version once he starts working on LLVM again (which should, presumably, happen soon). The reason why I'd like to see pushing forward is that we can backport the "fix" to the 4.0 branch. Eli, WDYT?

[efriedma]

Why do you want to backport this to 4.0? LoadCombine isn't part of the default pass pipeline.

test/Transforms/LoadCombine/load-combine-aa.ll

48

Check for the second load after the store?

[davide]

In D28922#660788, @efriedma wrote:

Why do you want to backport this to 4.0? LoadCombine isn't part of the default pass pipeline.

While the pass is not part of the default pipeline, some people use it (and apparently breaks).
That said, on a second thought, I decide to withdraw my request to backport this as it doesn't seem there's a reason strong enough to do that.
Also, the crash has been there since forever as far as I can tell, in fact I can reproduce it pre 3.9.

[Ka-Ka]

Any news about this issue?

test/Transforms/LoadCombine/load-combine-aa.ll

48

The testcase run first the load-combine pass and then instcombine pass. The instcombine pass will remove the second load after the store. If you think it is important to check for the second load then the testcase need to be splitted from the other tests and the instcombine pass need to be turned off.

[efriedma]

test/Transforms/LoadCombine/load-combine-aa.ll

48

Probably the whole file should be fixed not to run instcombine.

[davide]

Are you still working on this?

Cheers,

Davide

lib/Transforms/Scalar/LoadCombine.cpp

bool LoadCombine::runOnBasicBlock(BasicBlock &BB) {
   Builder = &TheBuilder;
 
   DenseMap<const Value *, SmallVector<LoadPOPPair, 8>> LoadMap;
   AliasSetTracker AST(*AA);
 
   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;
     ++NumLoadsAnalyzed;
     if (!LI->isSimple() || !LI->getType()->isIntegerTy())
       continue;
     auto POP = getPointerOffsetPair(*LI);
     if (!POP.Pointer)

test/Transforms/LoadCombine/load-combine-aa.ll

; CHECK: ret i64
   store i32 %load1, i32* %b, align 4
   %load2 = load i32, i32* %arrayidx1, align 4
   %conv2 = zext i32 %load2 to i64
   %shl = shl nuw i64 %conv2, 32
   %add = or i64 %shl, %conv
   ret i64 %add
 }
 
+%rec11 = type { i16, i16, i16 }
+@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
+}