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

Assertion failure in TwoAddressInstructionPass #97533

Closed
nikic opened this issue Jul 3, 2024 · 9 comments
Closed

Assertion failure in TwoAddressInstructionPass #97533

nikic opened this issue Jul 3, 2024 · 9 comments

Comments

@nikic
Copy link
Contributor

nikic commented Jul 3, 2024

; RUN: llc < %s
target triple = "x86_64-unknown-linux-gnu"

define ptr @test(ptr %ptr, i8 %arg, i1 %cond) {
entry:
  br i1 %cond, label %if, label %exit

if:
  %idx = zext i8 %arg to i64
  %gep1 = getelementptr ptr, ptr %ptr, i64 %idx
  %ptr2 = load ptr, ptr %gep1, align 8
  %gep2 = getelementptr i64, ptr %ptr2, i64 %idx
  br label %exit

exit:
  %phi = phi ptr [ %gep2, %if ], [ null, %entry ]
  ret ptr %phi
}

Results in:

llc: /home/npopov/repos/llvm-project/llvm/lib/CodeGen/TwoAddressInstructionPass.cpp:1461: bool (anonymous namespace)::TwoAddressInstructionPass::collectTiedOperands(MachineInstr *, TiedOperandMap &): Assertion `SrcReg && SrcMO.isUse() && "two address instruction invalid"' failed.
@llvmbot
Copy link
Collaborator

llvmbot commented Jul 3, 2024

@llvm/issue-subscribers-backend-x86

Author: Nikita Popov (nikic)

```llvm ; RUN: llc < %s target triple = "x86_64-unknown-linux-gnu"

define ptr @test(ptr %ptr, i8 %arg, i1 %cond) {
entry:
br i1 %cond, label %if, label %exit

if:
%idx = zext i8 %arg to i64
%gep1 = getelementptr ptr, ptr %ptr, i64 %idx
%ptr2 = load ptr, ptr %gep1, align 8
%gep2 = getelementptr i64, ptr %ptr2, i64 %idx
br label %exit

exit:
%phi = phi ptr [ %gep2, %if ], [ null, %entry ]
ret ptr %phi
}


Results in:

llc: /home/npopov/repos/llvm-project/llvm/lib/CodeGen/TwoAddressInstructionPass.cpp:1461: bool (anonymous namespace)::TwoAddressInstructionPass::collectTiedOperands(MachineInstr *, TiedOperandMap &): Assertion `SrcReg && SrcMO.isUse() && "two address instruction invalid"' failed.

</details>

@nikic
Copy link
Contributor Author

nikic commented Jul 3, 2024

The instruction in question is:

  %0:gr64 = ADD64rm $noreg(tied-def 0), killed %2:gr64, 8, killed %10:gr64_nosp, 0, $noreg, implicit-def dead $eflags :: (load (s64) from %ir.gep1)

@KanRobert
Copy link
Contributor

I will take a look.

@KanRobert
Copy link
Contributor

It started fail since llc 18.1 https://www.godbolt.org/z/v5KPn3G61

@ivan-shrimp
Copy link

Look like the branch doesn't matter: https://www.godbolt.org/z/hbrfc5TKs

@KanRobert
Copy link
Contributor

KanRobert commented Jul 4, 2024

It seems related to the optimization about folding shl op in X86DAGToDAGISel::matchAddressRecursively.

Here is a attempt

bash$ git diff
diff --git a/llvm/lib/Target/X86/X86ISelDAGToDAG.cpp b/llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
index f2b3855ebc54..fedd5e2babc2 100644
--- a/llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
+++ b/llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
@@ -2745,7 +2745,7 @@ bool X86DAGToDAGISel::matchAddressRecursively(SDValue N, X86ISelAddressMode &AM,
         Src = Src.getOperand(0);
       }

-    if (Src.getOpcode() == ISD::SHL && Src.hasOneUse()) {
+    if (Src.getOpcode() == ISD::SHL && Src.hasOneUse() && false) {
       // Give up if the shift is not a valid scale factor [1,2,3].
       SDValue ShlSrc = Src.getOperand(0);
       SDValue ShlAmt = Src.getOperand(1);

or

diff --git a/llvm/lib/Target/X86/X86ISelDAGToDAG.cpp b/llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
index f2b3855ebc54..89df407cf739 100644
--- a/llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
+++ b/llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
@@ -2784,7 +2784,7 @@ bool X86DAGToDAGISel::matchAddressRecursively(SDValue N, X86ISelAddressMode &AM,
       SDValue NewShl = CurDAG->getNode(ISD::SHL, DL, VT, Zext, ShlAmt);
       insertDAGNode(*CurDAG, N, NewShl);
       CurDAG->ReplaceAllUsesWith(N, NewShl);
-      CurDAG->RemoveDeadNode(N.getNode());
+      //CurDAG->RemoveDeadNode(N.getNode());

can make the test pass.

@KanRobert
Copy link
Contributor

@RKSimon Do you have interest to take a look? I see you touched this code in 7428739

@RKSimon
Copy link
Collaborator

RKSimon commented Jul 4, 2024

Commenting out this code seems to fix it - I'm trying to work out why.

if (OpOpcode == ISD::ZERO_EXTEND) { // (zext (zext x)) -> (zext x)
SDNodeFlags Flags;
Flags.setNonNeg(N1->getFlags().hasNonNeg());
return getNode(ISD::ZERO_EXTEND, DL, VT, N1.getOperand(0), Flags);
}

EDIT Only works for the original test case - not the branchless reduction

@RKSimon RKSimon self-assigned this Jul 4, 2024
@RKSimon
Copy link
Collaborator

RKSimon commented Jul 4, 2024

I have a workaround while I work on the underlying issue (we're deleting nodes that the DAG has already recorded in code selection)

@RKSimon RKSimon closed this as completed in e975ff0 Jul 4, 2024
DianQK pushed a commit to DianQK/llvm-project that referenced this issue Jul 5, 2024
…(x),c)) if the pattern has multiple uses

Fixes llvm#97533 crash where we hit a case where the root node had referenced the original zext node, which we then deleted - hopefully I can come up with a better solution, but the codegen changes don't look too bad atm (pulls out a shift from some complex LEA nodes that shared the scaled index).

(cherry picked from commit e975ff0)
nikic pushed a commit to rust-lang/llvm-project that referenced this issue Jul 5, 2024
…(x),c)) if the pattern has multiple uses

Fixes llvm#97533 crash where we hit a case where the root node had referenced the original zext node, which we then deleted - hopefully I can come up with a better solution, but the codegen changes don't look too bad atm (pulls out a shift from some complex LEA nodes that shared the scaled index).

(cherry picked from commit e975ff0)
kbluck pushed a commit to kbluck/llvm-project that referenced this issue Jul 6, 2024
…(x),c)) if the pattern has multiple uses

Fixes llvm#97533 crash where we hit a case where the root node had referenced the original zext node, which we then deleted - hopefully I can come up with a better solution, but the codegen changes don't look too bad atm (pulls out a shift from some complex LEA nodes that shared the scaled index).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants