[llvm] c593bf5 - [GlobalISel] Don't combine instructions which are fed by memory instructions.

[Jessica Paquette via llvm-commits]

Author: Jessica Paquette
Date: 2020-05-27T12:48:58-07:00
New Revision: c593bf534222f2206f89b6a61993125b2475b954

URL: https://github.com/llvm/llvm-project/commit/c593bf534222f2206f89b6a61993125b2475b954
DIFF: https://github.com/llvm/llvm-project/commit/c593bf534222f2206f89b6a61993125b2475b954.diff

LOG: [GlobalISel] Don't combine instructions which are fed by memory instructions.

If we have a memory instruction (e.g. a load), we shouldn't combine it away in
some trivial combine.

It's possible that, say, a call lives between the instructions. This could
modify the value loaded, making the load instructions not safe to fold.

Differential Revision: https://reviews.llvm.org/D80053

Added: 
    llvm/test/CodeGen/AArch64/GlobalISel/prelegalizercombiner-not-really-equiv-insts.mir

Modified: 
    llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp b/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
index 1d888245af9f..45b7d991ae72 100644
--- a/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
@@ -1534,8 +1534,28 @@ bool CombinerHelper::matchEqualDefs(const MachineOperand &MOP1,
   if (!I2)
     return false;
 
-  // Check for physical registers on the instructions first to avoid cases like
-  // this:
+  // If we have an instruction which loads or stores, we can't guarantee that
+  // it is identical.
+  //
+  // For example, we may have
+  //
+  // %x1 = G_LOAD %addr (load N from @somewhere)
+  // ...
+  // call @foo
+  // ...
+  // %x2 = G_LOAD %addr (load N from @somewhere)
+  // ...
+  // %or = G_OR %x1, %x2
+  //
+  // It's possible that @foo will modify whatever lives at the address we're
+  // loading from. To be safe, let's just assume that all loads and stores
+  // are 
diff erent (unless we have something which is guaranteed to not
+  // change.)
+  if (I1->mayLoadOrStore() && !I1->isDereferenceableInvariantLoad(nullptr))
+    return false;
+
+  // Check for physical registers on the instructions first to avoid cases
+  // like this:
   //
   // %a = COPY $physreg
   // ...

diff  --git a/llvm/test/CodeGen/AArch64/GlobalISel/prelegalizercombiner-not-really-equiv-insts.mir b/llvm/test/CodeGen/AArch64/GlobalISel/prelegalizercombiner-not-really-equiv-insts.mir
new file mode 100644
index 000000000000..e387c5e58d6f
--- /dev/null
+++ b/llvm/test/CodeGen/AArch64/GlobalISel/prelegalizercombiner-not-really-equiv-insts.mir
@@ -0,0 +1,82 @@
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -mtriple aarch64 -run-pass=aarch64-prelegalizer-combiner -verify-machineinstrs %s -o - | FileCheck %s
+
+--- |
+  @g = external hidden unnamed_addr global i32, align 4
+  define void @not_necessarily_equiv_loads() { ret void }
+  define void @invariant_loads() { ret void }
+  define void @both_have_to_be_invariant() { ret void }
+...
+---
+name:            not_necessarily_equiv_loads
+tracksRegLiveness: true
+machineFunctionInfo: {}
+body:             |
+  bb.0:
+
+    ; %load1 || %load2 == %load1 is not necessarily true, even though they
+    ; both load from the same address. Whatever is in that address may be
+    ; changed by another instruction which appears between them.
+    ;
+    ; Check that we don't remove the G_OR.
+
+    ; CHECK-LABEL: name: not_necessarily_equiv_loads
+    ; CHECK: %ptr:_(p0) = G_GLOBAL_VALUE @g
+    ; CHECK: %load1:_(s32) = G_LOAD %ptr(p0) :: (load 4 from @g)
+    ; CHECK: %load2:_(s32) = G_LOAD %ptr(p0) :: (load 4 from @g)
+    ; CHECK: %or:_(s32) = G_OR %load2, %load1
+    ; CHECK: G_STORE %or(s32), %ptr(p0) :: (store 4 into @g)
+    ; CHECK: RET_ReallyLR
+    %ptr:_(p0) = G_GLOBAL_VALUE @g
+    %load1:_(s32) = G_LOAD %ptr(p0) :: (load 4 from @g)
+    %load2:_(s32) = G_LOAD %ptr(p0) :: (load 4 from @g)
+    %or:_(s32) = G_OR %load2, %load1
+    G_STORE %or(s32), %ptr(p0) :: (store 4 into @g)
+    RET_ReallyLR
+
+...
+---
+name:            invariant_loads
+tracksRegLiveness: true
+machineFunctionInfo: {}
+body:             |
+  bb.0:
+
+    ; %load1 || %load2 == %load1 is fine here, because the loads are invariant.
+
+    ; CHECK-LABEL: name: invariant_loads
+    ; CHECK: %ptr:_(p0) = G_GLOBAL_VALUE @g
+    ; CHECK: %load2:_(s32) = G_LOAD %ptr(p0) :: (dereferenceable invariant load 4 from @g)
+    ; CHECK: G_STORE %load2(s32), %ptr(p0) :: (store 4 into @g)
+    ; CHECK: RET_ReallyLR
+    %ptr:_(p0) = G_GLOBAL_VALUE @g
+    %load1:_(s32) = G_LOAD %ptr(p0) :: (dereferenceable invariant load 4 from @g)
+    %load2:_(s32) = G_LOAD %ptr(p0) :: (dereferenceable invariant load 4 from @g)
+    %or:_(s32) = G_OR %load2, %load1
+    G_STORE %or(s32), %ptr(p0) :: (store 4 into @g)
+    RET_ReallyLR
+
+...
+---
+name:            both_have_to_be_invariant
+tracksRegLiveness: true
+machineFunctionInfo: {}
+body:             |
+  bb.0:
+
+    ; We shouldn't combine here, because the loads both have to be invariant.
+
+    ; CHECK-LABEL: name: both_have_to_be_invariant
+    ; CHECK: %ptr:_(p0) = G_GLOBAL_VALUE @g
+    ; CHECK: %load1:_(s32) = G_LOAD %ptr(p0) :: (dereferenceable invariant load 4 from @g)
+    ; CHECK: %load2:_(s32) = G_LOAD %ptr(p0) :: (dereferenceable load 4 from @g)
+    ; CHECK: %or:_(s32) = G_OR %load2, %load1
+    ; CHECK: G_STORE %or(s32), %ptr(p0) :: (store 4 into @g)
+    ; CHECK: RET_ReallyLR
+    %ptr:_(p0) = G_GLOBAL_VALUE @g
+    %load1:_(s32) = G_LOAD %ptr(p0) :: (dereferenceable invariant load 4 from @g)
+    %load2:_(s32) = G_LOAD %ptr(p0) :: (dereferenceable load 4 from @g)
+    %or:_(s32) = G_OR %load2, %load1
+    G_STORE %or(s32), %ptr(p0) :: (store 4 into @g)
+    RET_ReallyLR
+...