Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GlobalISel] Make sure to check for load barriers when merging G_EXTRACT_VECTOR_ELT into G_LOAD. #82306

Merged
merged 1 commit into from
Feb 21, 2024

Conversation

resistor
Copy link
Collaborator

Fixes #78477

@llvmbot
Copy link
Collaborator

llvmbot commented Feb 20, 2024

@llvm/pr-subscribers-llvm-globalisel

@llvm/pr-subscribers-backend-aarch64

Author: Owen Anderson (resistor)

Changes

Fixes #78477


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

2 Files Affected:

  • (modified) llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp (+12)
  • (added) llvm/test/CodeGen/AArch64/extractvector-of-load.mir (+91)
diff --git a/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp b/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
index b400eb34e2901b..45427fc2466bd1 100644
--- a/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
@@ -1198,6 +1198,18 @@ bool CombinerHelper::matchCombineExtractedVectorLoad(MachineInstr &MI,
   if (!VecEltTy.isByteSized())
     return false;
 
+  // Check for load fold barriers between the extraction and the load.
+  if (MI.getParent() != LoadMI->getParent())
+    return false;
+  const unsigned MaxIter = 20;
+  unsigned Iter = 0;
+  for (auto II = LoadMI->getIterator(), IE = MI.getIterator(); II != IE; ++II) {
+    if (II->isLoadFoldBarrier())
+      return false;
+    if (Iter++ == MaxIter)
+      return false;
+  }
+
   // Check if the new load that we are going to create is legal
   // if we are in the post-legalization phase.
   MachineMemOperand MMO = LoadMI->getMMO();
diff --git a/llvm/test/CodeGen/AArch64/extractvector-of-load.mir b/llvm/test/CodeGen/AArch64/extractvector-of-load.mir
new file mode 100644
index 00000000000000..d4c28917300760
--- /dev/null
+++ b/llvm/test/CodeGen/AArch64/extractvector-of-load.mir
@@ -0,0 +1,91 @@
+# RUN: llc -run-pass=aarch64-prelegalizer-combiner %s -o - | FileCheck %s
+
+--- |
+  ; ModuleID = 'in.ll'
+  source_filename = "in.ll"
+  target datalayout = "e-m:o-i64:64-i128:128-n32:64-S128"
+  
+  define i32 @f(ptr %0) {
+    %2 = load <2 x i32>, ptr %0, align 8
+    store <4 x i32> zeroinitializer, ptr %0, align 16
+    %3 = extractelement <2 x i32> %2, i64 0
+    ret i32 %3
+  }
+
+...
+---
+name:            f
+alignment:       4
+exposesReturnsTwice: false
+legalized:       false
+regBankSelected: false
+selected:        false
+failedISel:      false
+tracksRegLiveness: true
+hasWinCFI:       false
+callsEHReturn:   false
+callsUnwindInit: false
+hasEHCatchret:   false
+hasEHScopes:     false
+hasEHFunclets:   false
+isOutlined:      false
+debugInstrRef:   false
+failsVerification: false
+tracksDebugUserValues: false
+registers:
+  - { id: 0, class: _, preferred-register: '' }
+  - { id: 1, class: _, preferred-register: '' }
+  - { id: 2, class: _, preferred-register: '' }
+  - { id: 3, class: _, preferred-register: '' }
+  - { id: 4, class: _, preferred-register: '' }
+  - { id: 5, class: _, preferred-register: '' }
+liveins:
+  - { reg: '$x0', virtual-reg: '' }
+frameInfo:
+  isFrameAddressTaken: false
+  isReturnAddressTaken: false
+  hasStackMap:     false
+  hasPatchPoint:   false
+  stackSize:       0
+  offsetAdjustment: 0
+  maxAlignment:    1
+  adjustsStack:    false
+  hasCalls:        false
+  stackProtector:  ''
+  functionContext: ''
+  maxCallFrameSize: 4294967295
+  cvBytesOfCalleeSavedRegisters: 0
+  hasOpaqueSPAdjustment: false
+  hasVAStart:      false
+  hasMustTailInVarArgFunc: false
+  hasTailCall:     false
+  localFrameSize:  0
+  savePoint:       ''
+  restorePoint:    ''
+fixedStack:      []
+stack:           []
+entry_values:    []
+callSites:       []
+debugValueSubstitutions: []
+constants:       []
+machineFunctionInfo: {}
+body:             |
+  bb.1 (%ir-block.1):
+    liveins: $x0
+  
+    %0:_(p0) = COPY $x0
+    %3:_(s32) = G_CONSTANT i32 0
+    %2:_(<4 x s32>) = G_BUILD_VECTOR %3(s32), %3(s32), %3(s32), %3(s32)
+    %5:_(s64) = G_CONSTANT i64 0
+    %1:_(<2 x s32>) = G_LOAD %0(p0) :: (load (<2 x s32>) from %ir.0)
+    G_STORE %2(<4 x s32>), %0(p0) :: (store (<4 x s32>) into %ir.0)
+    %4:_(s32) = G_EXTRACT_VECTOR_ELT %1(<2 x s32>), %5(s64)
+    $w0 = COPY %4(s32)
+    RET_ReallyLR implicit $w0
+
+...
+
+# CHECK: bb.0
+# CHECK: G_LOAD
+# CHECK: G_STORE
+# CHECK: G_EXTRACT_VECTOR_ELT

return false;
const unsigned MaxIter = 20;
unsigned Iter = 0;
for (auto II = LoadMI->getIterator(), IE = MI.getIterator(); II != IE; ++II) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

random aside: I didn't look too hard, but I'm surprised I couldn't find anything in STLExtras that automates this "do a think for a maximum number of iterations over a range" pattern

Like it would be cool to be able to write something like

if (any_of(make_bounded_range(LoadMI->getIterator(), MI.getIterator(), 20)
                              [const MachineInstr &Btwn]{
                                 return Btwn.isLoadFoldBarrier();
                              })
  return false;

But I don't know if that exists today.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not aware of an option.

Copy link

@ornata ornata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@resistor resistor merged commit c02b0d0 into llvm:main Feb 21, 2024
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AArch64 with global isel miscompile
4 participants