[llvm] [SDAG][X86] Merge MMOs when reusing morphed node (PR #206700)

[via llvm-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-llvm-selectiondag

@llvm/pr-subscribers-backend-x86

Author: Nikita Popov (nikic)

<details>
<summary>Changes</summary>

Multiple isel patterns may morph to the same node. Currently, it will retain the MMOs from whichever one happens to run last.

This doesn't work with X86's EnablePromoteAnyextLoad feature, which can select the same MOV32rm from an i32 load and an appropriately aligned anyexted i16 load. If we end up retaining only the s16 MMO and drop the s32 MMO, this can result in miscompiles due to incorrect reordering down the road.

Fix this by merging MMOs instead of only retaining the last ones.

I'm not familiar with this area, and not sure whether this is the right fix. Maybe X86 needs to be doing something special for EnablePromoteAnyextLoad instead?

Fixes https://github.com/llvm/llvm-project/issues/194853.

---
Full diff: https://github.com/llvm/llvm-project/pull/206700.diff


3 Files Affected:

- (modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp (+28-1) 
- (modified) llvm/test/CodeGen/X86/pr37916.ll (+2-2) 
- (added) llvm/test/CodeGen/X86/promote-anyext-load-mmo.ll (+24) 


``````````diff
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
index 5ae52cae771fb..5b4a581e0f0a7 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
@@ -4461,7 +4461,34 @@ void SelectionDAGISel::SelectCodeCommon(SDNode *NodeToMatch,
           }
         }
 
-        CurDAG->setNodeMemRefs(Res, FilteredMemRefs);
+        if (Res->memoperands_empty()) {
+          // If the node is new, just copy the MMOs.
+          CurDAG->setNodeMemRefs(Res, FilteredMemRefs);
+        } else {
+          // If the node is reused, most likely the existing MMOs are exactly
+          // the same as the new ones, in which case there is nothing to do.
+          bool AllEqual =
+              Res->memoperands().size() == FilteredMemRefs.size() &&
+              all_of(zip_equal(Res->memoperands(), FilteredMemRefs),
+                     [](const std::tuple<const MachineMemOperand *,
+                                         const MachineMemOperand *> &Pair) {
+                       return *std::get<0>(Pair) == *std::get<1>(Pair);
+                     });
+          if (!AllEqual) {
+            // If the MMOs differ, we have to retain both lists to be
+            // conservative. Deduplicate the MMOs.
+            SmallVector<MachineMemOperand *> NewMemRefs;
+            append_range(NewMemRefs, Res->memoperands());
+            for (MachineMemOperand *NewMMO : FilteredMemRefs) {
+              if (any_of(NewMemRefs, [NewMMO](MachineMemOperand *MMO) {
+                    return *MMO == *NewMMO;
+                  }))
+                continue;
+              NewMemRefs.push_back(NewMMO);
+            }
+            CurDAG->setNodeMemRefs(Res, NewMemRefs);
+          }
+        }
       }
 
       LLVM_DEBUG({
diff --git a/llvm/test/CodeGen/X86/pr37916.ll b/llvm/test/CodeGen/X86/pr37916.ll
index e6639a11ca5ea..96e7e866bf4b2 100644
--- a/llvm/test/CodeGen/X86/pr37916.ll
+++ b/llvm/test/CodeGen/X86/pr37916.ll
@@ -10,8 +10,8 @@ define void @fn1() local_unnamed_addr {
 ; CHECK-NEXT:    .p2align 4
 ; CHECK-NEXT:  .LBB0_1: # %if.end
 ; CHECK-NEXT:    # =>This Inner Loop Header: Depth=1
-; CHECK-NEXT:    movl a+4, %eax
-; CHECK-NEXT:    orl a, %eax
+; CHECK-NEXT:    movl a, %eax
+; CHECK-NEXT:    orl a+4, %eax
 ; CHECK-NEXT:    movl $a, f
 ; CHECK-NEXT:    je .LBB0_3
 ; CHECK-NEXT:  # %bb.2: # %if.end
diff --git a/llvm/test/CodeGen/X86/promote-anyext-load-mmo.ll b/llvm/test/CodeGen/X86/promote-anyext-load-mmo.ll
new file mode 100644
index 0000000000000..03077751b6ed4
--- /dev/null
+++ b/llvm/test/CodeGen/X86/promote-anyext-load-mmo.ll
@@ -0,0 +1,24 @@
+; NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 6
+; RUN: llc -mtriple=x86_64-- -stop-after=x86-isel < %s | FileCheck %s
+
+declare void @use(i32, i16)
+
+; Should retain both the s32 and s16 MMOs here.
+define void @test(ptr %p) nounwind {
+  ; CHECK-LABEL: name: test
+  ; CHECK: bb.0 (%ir-block.0):
+  ; CHECK-NEXT:   liveins: $rdi
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT:   [[COPY:%[0-9]+]]:gr64 = COPY $rdi
+  ; CHECK-NEXT:   [[MOV32rm:%[0-9]+]]:gr32 = MOV32rm [[COPY]], 1, $noreg, 0, $noreg :: (load (s32) from %ir.p), (load (s16) from %ir.p, align 4)
+  ; CHECK-NEXT:   ADJCALLSTACKDOWN64 0, 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
+  ; CHECK-NEXT:   $edi = COPY [[MOV32rm]]
+  ; CHECK-NEXT:   $esi = COPY [[MOV32rm]]
+  ; CHECK-NEXT:   CALL64pcrel32 target-flags(x86-plt) @use, csr_64, implicit $rsp, implicit $ssp, implicit $edi, implicit $esi, implicit-def $rsp, implicit-def $ssp
+  ; CHECK-NEXT:   ADJCALLSTACKUP64 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
+  ; CHECK-NEXT:   RET 0
+  %x = load i32, ptr %p
+  %y = load i16, ptr %p, align 4
+  call void @use(i32 %x, i16 %y)
+  ret void
+}

``````````

</details>


https://github.com/llvm/llvm-project/pull/206700