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

[PowerPC] Fix assert exposed by PR 95931 in LowerBITCAST #108062

Merged
merged 3 commits into from
Sep 10, 2024

Conversation

syzaara
Copy link
Contributor

@syzaara syzaara commented Sep 10, 2024

Hit Assertion failed: Num < NumOperands && "Invalid child # of SDNode!"
Fix by checking opcode and value type before calling getOperand.

Hit Assertion failed: Num < NumOperands && "Invalid child # of SDNode!"
Check opcode and value type before calling getOperand.
@llvmbot
Copy link
Collaborator

llvmbot commented Sep 10, 2024

@llvm/pr-subscribers-backend-powerpc

Author: Zaara Syeda (syzaara)

Changes

Hit Assertion failed: Num < NumOperands && "Invalid child # of SDNode!" Check opcode and value type before calling getOperand.


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

2 Files Affected:

  • (modified) llvm/lib/Target/PowerPC/PPCISelLowering.cpp (+5-4)
  • (modified) llvm/test/CodeGen/PowerPC/f128-bitcast.ll (+22)
diff --git a/llvm/lib/Target/PowerPC/PPCISelLowering.cpp b/llvm/lib/Target/PowerPC/PPCISelLowering.cpp
index fd03eeba911490..ed63b50cd8eeea 100644
--- a/llvm/lib/Target/PowerPC/PPCISelLowering.cpp
+++ b/llvm/lib/Target/PowerPC/PPCISelLowering.cpp
@@ -9470,12 +9470,13 @@ SDValue PPCTargetLowering::LowerBITCAST(SDValue Op, SelectionDAG &DAG) const {
   SDLoc dl(Op);
   SDValue Op0 = Op->getOperand(0);
 
+  if ((Op.getValueType() != MVT::f128) || (Op0.getOpcode() != ISD::BUILD_PAIR))
+    return SDValue();
+
   SDValue Lo = Op0.getOperand(0);
   SDValue Hi = Op0.getOperand(1);
-
-  if ((Op.getValueType() != MVT::f128) ||
-      (Op0.getOpcode() != ISD::BUILD_PAIR) || (Lo.getValueType() != MVT::i64) ||
-      (Hi.getValueType() != MVT::i64) || !Subtarget.isPPC64())
+  if ((Lo.getValueType() != MVT::i64) || (Hi.getValueType() != MVT::i64) ||
+      !Subtarget.isPPC64())
     return SDValue();
 
   if (!Subtarget.isLittleEndian())
diff --git a/llvm/test/CodeGen/PowerPC/f128-bitcast.ll b/llvm/test/CodeGen/PowerPC/f128-bitcast.ll
index ffbfbd0c64ff3f..649ca59fe9ae03 100644
--- a/llvm/test/CodeGen/PowerPC/f128-bitcast.ll
+++ b/llvm/test/CodeGen/PowerPC/f128-bitcast.ll
@@ -86,3 +86,25 @@ entry:
   ret i64 %1
 }
 
+define <4 x i32> @test(i512 %a) {
+; CHECK-LABEL: test:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    mtvsrdd v2, r4, r3
+; CHECK-NEXT:    blr
+;
+; CHECK-BE-LABEL: test:
+; CHECK-BE:       # %bb.0: # %entry
+; CHECK-BE-NEXT:    mtvsrdd v2, r9, r10
+; CHECK-BE-NEXT:    blr
+;
+; CHECK-P8-LABEL: test:
+; CHECK-P8:       # %bb.0: # %entry
+; CHECK-P8-NEXT:    mtfprd f0, r3
+; CHECK-P8-NEXT:    mtfprd f1, r4
+; CHECK-P8-NEXT:    xxmrghd v2, vs1, vs0
+; CHECK-P8-NEXT:    blr
+entry:
+  %0 = trunc i512 %a to i128
+  %1 = bitcast i128 %0 to <4 x i32>
+  ret <4 x i32> %1
+}

Copy link
Contributor

@lei137 lei137 left a comment

Choose a reason for hiding this comment

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

LGTM
Thx!

(Op0.getOpcode() != ISD::BUILD_PAIR) || (Lo.getValueType() != MVT::i64) ||
(Hi.getValueType() != MVT::i64) || !Subtarget.isPPC64())
if ((Lo.getValueType() != MVT::i64) || (Hi.getValueType() != MVT::i64) ||
!Subtarget.isPPC64())
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe good to move !Subtarget.isPPC64() to begining of cond on line 9473.

Copy link
Contributor

@amy-kwan amy-kwan left a comment

Choose a reason for hiding this comment

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

LGTM.

@@ -86,3 +86,25 @@ entry:
ret i64 %1
}

define <4 x i32> @test(i512 %a) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Can we maybe be a bit more descriptive for the test title? Not sure if something like truncBitcast makes sense, or something like that?

@syzaara syzaara merged commit 22067a8 into llvm:main Sep 10, 2024
4 of 6 checks passed
@tgross35
Copy link

tgross35 commented Sep 10, 2024

This should probably get backported since #95931 was in #95931 (or I guess that one reverted, since I said in the backport that it seemed low risk but that was apparently erroneous :/)

@syzaara
Copy link
Contributor Author

syzaara commented Sep 10, 2024

/cherry-pick 22067a8

@llvmbot
Copy link
Collaborator

llvmbot commented Sep 10, 2024

/cherry-pick 22067a8

Error: Command failed due to missing milestone.

@amy-kwan amy-kwan added this to the LLVM 19.X Release milestone Sep 10, 2024
@syzaara
Copy link
Contributor Author

syzaara commented Sep 10, 2024

This should probably get backported since #95931 was in #95931 (or I guess that one reverted, since I said in the backport that it seemed low risk but that was apparently erroneous :/)

I see 95931 change is still in release/19.x so it would be good to have this PR back ported as well.

VitaNuo pushed a commit to VitaNuo/llvm-project that referenced this pull request Sep 12, 2024
Hit Assertion failed: Num < NumOperands && "Invalid child # of SDNode!" 
Fix by checking opcode and value type before calling getOperand.
@syzaara
Copy link
Contributor Author

syzaara commented Sep 16, 2024

/cherry-pick 22067a8

llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Sep 16, 2024
Hit Assertion failed: Num < NumOperands && "Invalid child # of SDNode!"
Fix by checking opcode and value type before calling getOperand.

(cherry picked from commit 22067a8)
@llvmbot
Copy link
Collaborator

llvmbot commented Sep 16, 2024

/pull-request #108834

tru pushed a commit to llvmbot/llvm-project that referenced this pull request Sep 16, 2024
Hit Assertion failed: Num < NumOperands && "Invalid child # of SDNode!"
Fix by checking opcode and value type before calling getOperand.

(cherry picked from commit 22067a8)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

5 participants