[GlobalISel] Miscompilation: G_SELECT got combined by select_same_val for different values

[agostonrobert]

I am new to LLVM, and I am working on a downstream port with GlobalISel, and a customer reported the issue that for the following simple C code the compiler ignores the type cast.

unsigned test(int a, int b, int c, int d, unsigned short x) {
  return a ? ((unsigned char) x) : x;
}

I tried to track down the issue and come up with the following minimal example that fails the same way also with AArch64:

# 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

---
name:            select_same_val
fixedStack:
  - { id: 0, type: default, offset: 8, size: 4, alignment: 8, stack-id: default,
      isImmutable: true, isAliased: false, callee-saved-register: '', callee-saved-restored: true,
      debug-info-variable: '', debug-info-expression: '', debug-info-location: '' }
body:             |
  bb.0.entry:
    ; CHECK-LABEL: name: select_same_val
    ; CHECK: [[FRAME_INDEX:%[0-9]+]]:_(p0) = G_FRAME_INDEX %fixed-stack.0
    ; CHECK-NEXT: [[COPY:%[0-9]+]]:_(s32) = COPY $w0
    ; CHECK-NEXT: [[ZEXTLOAD:%[0-9]+]]:_(s32) = G_ZEXTLOAD [[FRAME_INDEX]](p0) :: (load (s8) from %fixed-stack.0)
    ; CHECK-NEXT: [[ZEXTLOAD1:%[0-9]+]]:_(s32) = G_ZEXTLOAD [[FRAME_INDEX]](p0) :: (load (s16) from %fixed-stack.0)
    ; CHECK-NEXT: [[SELECT:%[0-9]+]]:_(s32) = G_SELECT [[COPY]](s32), [[ZEXTLOAD]], [[ZEXTLOAD1]]
    ; CHECK-NEXT: $w0 = COPY [[SELECT]](s32)
    ; CHECK-NEXT: RET_ReallyLR
    %0:_(p0) = G_FRAME_INDEX %fixed-stack.0
    %1:_(s32) = COPY $w0

    %2:_(s32) = G_ZEXTLOAD %0:_(p0) :: (load (s8) from %fixed-stack.0)
    %3:_(s32) = G_ZEXTLOAD %0:_(p0) :: (load (s16) from %fixed-stack.0)

    %6:_(s32) = G_SELECT %1:_(s32), %2:_, %3:_

    $w0 = COPY %6:_(s32)
    RET_ReallyLR
...

My understanding is that the select_same_val in Combine.td assumes that values loaded by the two G_ZEXTLOAD are the same, which is not true as they have different sizes.
I am not sure if the problem is with select_same_val, or something is missing from the loads.

[llvmbot]

@llvm/issue-subscribers-backend-aarch64

[agostonrobert]

Before I figure out how to propose fixes properly, I just leave my prototype here as a comment:

diff --git a/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp b/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
index d6a009744161..732102b4137d 100644
--- a/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
@@ -2350,6 +2350,19 @@ bool CombinerHelper::matchEqualDefs(const MachineOperand &MOP1,
   if (I1->mayLoadOrStore() && !I1->isDereferenceableInvariantLoad(nullptr))
     return false;
 
+  // If both instructions are loads or stores, they are equal only if both
+  // are derefereneable invariant loads with the same number of bits.
+  if (I1->mayLoadOrStore() && I2->mayLoadOrStore()) {
+    GLoadStore *LoadStoreMI1 = dyn_cast<GLoadStore>(I1);
+    GLoadStore *LoadStoreMI2 = dyn_cast<GLoadStore>(I2);
+    if (LoadStoreMI1 && LoadStoreMI2) {
+      if (!I2->isDereferenceableInvariantLoad(nullptr) ||
+          (LoadStoreMI1->getMemSizeInBits() !=
+           LoadStoreMI2->getMemSizeInBits()))
+        return false;
+    }
+  }
+
   // Check for physical registers on the instructions first to avoid cases
   // like this:
   //

[agostonrobert]

Proposed fix is at https://reviews.llvm.org/D118805