[RISCV][GISel] Fallback in LowerCall for byval arguments. #119251
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Our byval call lowering isn't copying the argument. Looks like the SelectionDAG code for byval is different than AArch64 so this may be non-trivial to fix. Reject for now.
|
@llvm/pr-subscribers-backend-risc-v Author: Craig Topper (topperc) ChangesOur byval call lowering isn't copying the argument. Looks like our SelectionDAG code for byval is different than AArch64 so this may be non-trivial to fix. Reject for now. Full diff: https://github.com/llvm/llvm-project/pull/119251.diff 3 Files Affected:
diff --git a/llvm/lib/Target/RISCV/GISel/RISCVCallLowering.cpp b/llvm/lib/Target/RISCV/GISel/RISCVCallLowering.cpp
index 800f886dd70830..e451f535809c20 100644
--- a/llvm/lib/Target/RISCV/GISel/RISCVCallLowering.cpp
+++ b/llvm/lib/Target/RISCV/GISel/RISCVCallLowering.cpp
@@ -596,6 +596,8 @@ bool RISCVCallLowering::lowerCall(MachineIRBuilder &MIRBuilder,
for (auto &AInfo : Info.OrigArgs) {
if (!isSupportedArgumentType(AInfo.Ty, Subtarget))
return false;
+ if (AInfo.Flags[0].isByVal())
+ return false;
}
if (!Info.OrigRet.Ty->isVoidTy() &&
diff --git a/llvm/test/CodeGen/RISCV/GlobalISel/irtranslator/calls.ll b/llvm/test/CodeGen/RISCV/GlobalISel/irtranslator/calls.ll
index b06b539ded19b9..34e29718d0e9a6 100644
--- a/llvm/test/CodeGen/RISCV/GlobalISel/irtranslator/calls.ll
+++ b/llvm/test/CodeGen/RISCV/GlobalISel/irtranslator/calls.ll
@@ -350,31 +350,6 @@ entry:
%struct.Foo = type { i32, i32, i32, i16, i8 }
@foo = global %struct.Foo { i32 1, i32 2, i32 3, i16 4, i8 5 }, align 4
-declare void @void_byval_args(ptr byval(%struct.Foo) %f)
-
-define void @test_void_byval_args() {
- ; RV32I-LABEL: name: test_void_byval_args
- ; RV32I: bb.1.entry:
- ; RV32I-NEXT: [[GV:%[0-9]+]]:_(p0) = G_GLOBAL_VALUE @foo
- ; RV32I-NEXT: ADJCALLSTACKDOWN 0, 0, implicit-def $x2, implicit $x2
- ; RV32I-NEXT: $x10 = COPY [[GV]](p0)
- ; RV32I-NEXT: PseudoCALL target-flags(riscv-call) @void_byval_args, csr_ilp32_lp64, implicit-def $x1, implicit $x10
- ; RV32I-NEXT: ADJCALLSTACKUP 0, 0, implicit-def $x2, implicit $x2
- ; RV32I-NEXT: PseudoRET
- ;
- ; RV64I-LABEL: name: test_void_byval_args
- ; RV64I: bb.1.entry:
- ; RV64I-NEXT: [[GV:%[0-9]+]]:_(p0) = G_GLOBAL_VALUE @foo
- ; RV64I-NEXT: ADJCALLSTACKDOWN 0, 0, implicit-def $x2, implicit $x2
- ; RV64I-NEXT: $x10 = COPY [[GV]](p0)
- ; RV64I-NEXT: PseudoCALL target-flags(riscv-call) @void_byval_args, csr_ilp32_lp64, implicit-def $x1, implicit $x10
- ; RV64I-NEXT: ADJCALLSTACKUP 0, 0, implicit-def $x2, implicit $x2
- ; RV64I-NEXT: PseudoRET
-entry:
- call void @void_byval_args(ptr byval(%struct.Foo) @foo)
- ret void
-}
-
declare void @void_sret_args(ptr sret(%struct.Foo) %f)
define void @test_void_sret_args() {
diff --git a/llvm/test/CodeGen/RISCV/GlobalISel/riscv-unsupported.ll b/llvm/test/CodeGen/RISCV/GlobalISel/riscv-unsupported.ll
new file mode 100644
index 00000000000000..073b5186aad718
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/GlobalISel/riscv-unsupported.ll
@@ -0,0 +1,16 @@
+; RUN: llc -mtriple=riscv32 -verify-machineinstrs -global-isel -global-isel-abort=2 -pass-remarks-missed='gisel*' %s -o - 2>&1 | FileCheck %s -check-prefixes=CHECK
+; RUN: llc -mtriple=riscv64 -verify-machineinstrs -global-isel -global-isel-abort=2 -pass-remarks-missed='gisel*' %s -o - 2>&1 | FileCheck %s -check-prefixes=CHECK
+
+; This file checks that we use the fallback path for things that are known to
+; be unsupported on the RISC-V target. It should progressively shrink in size.
+
+%byval.class = type { i32 }
+
+declare void @test_byval_arg(ptr byval(%byval.class) %x)
+
+define void @test_byval_param(ptr %x) {
+; CHECK: remark: {{.*}} unable to translate instruction: call
+; CHECK-LABEL: warning: Instruction selection used fallback path for test_byval_param
+ call void @test_byval_arg(ptr byval(%byval.class) %x)
+ ret void
+}
|
|
@llvm/pr-subscribers-llvm-globalisel Author: Craig Topper (topperc) ChangesOur byval call lowering isn't copying the argument. Looks like our SelectionDAG code for byval is different than AArch64 so this may be non-trivial to fix. Reject for now. Full diff: https://github.com/llvm/llvm-project/pull/119251.diff 3 Files Affected:
diff --git a/llvm/lib/Target/RISCV/GISel/RISCVCallLowering.cpp b/llvm/lib/Target/RISCV/GISel/RISCVCallLowering.cpp
index 800f886dd70830..e451f535809c20 100644
--- a/llvm/lib/Target/RISCV/GISel/RISCVCallLowering.cpp
+++ b/llvm/lib/Target/RISCV/GISel/RISCVCallLowering.cpp
@@ -596,6 +596,8 @@ bool RISCVCallLowering::lowerCall(MachineIRBuilder &MIRBuilder,
for (auto &AInfo : Info.OrigArgs) {
if (!isSupportedArgumentType(AInfo.Ty, Subtarget))
return false;
+ if (AInfo.Flags[0].isByVal())
+ return false;
}
if (!Info.OrigRet.Ty->isVoidTy() &&
diff --git a/llvm/test/CodeGen/RISCV/GlobalISel/irtranslator/calls.ll b/llvm/test/CodeGen/RISCV/GlobalISel/irtranslator/calls.ll
index b06b539ded19b9..34e29718d0e9a6 100644
--- a/llvm/test/CodeGen/RISCV/GlobalISel/irtranslator/calls.ll
+++ b/llvm/test/CodeGen/RISCV/GlobalISel/irtranslator/calls.ll
@@ -350,31 +350,6 @@ entry:
%struct.Foo = type { i32, i32, i32, i16, i8 }
@foo = global %struct.Foo { i32 1, i32 2, i32 3, i16 4, i8 5 }, align 4
-declare void @void_byval_args(ptr byval(%struct.Foo) %f)
-
-define void @test_void_byval_args() {
- ; RV32I-LABEL: name: test_void_byval_args
- ; RV32I: bb.1.entry:
- ; RV32I-NEXT: [[GV:%[0-9]+]]:_(p0) = G_GLOBAL_VALUE @foo
- ; RV32I-NEXT: ADJCALLSTACKDOWN 0, 0, implicit-def $x2, implicit $x2
- ; RV32I-NEXT: $x10 = COPY [[GV]](p0)
- ; RV32I-NEXT: PseudoCALL target-flags(riscv-call) @void_byval_args, csr_ilp32_lp64, implicit-def $x1, implicit $x10
- ; RV32I-NEXT: ADJCALLSTACKUP 0, 0, implicit-def $x2, implicit $x2
- ; RV32I-NEXT: PseudoRET
- ;
- ; RV64I-LABEL: name: test_void_byval_args
- ; RV64I: bb.1.entry:
- ; RV64I-NEXT: [[GV:%[0-9]+]]:_(p0) = G_GLOBAL_VALUE @foo
- ; RV64I-NEXT: ADJCALLSTACKDOWN 0, 0, implicit-def $x2, implicit $x2
- ; RV64I-NEXT: $x10 = COPY [[GV]](p0)
- ; RV64I-NEXT: PseudoCALL target-flags(riscv-call) @void_byval_args, csr_ilp32_lp64, implicit-def $x1, implicit $x10
- ; RV64I-NEXT: ADJCALLSTACKUP 0, 0, implicit-def $x2, implicit $x2
- ; RV64I-NEXT: PseudoRET
-entry:
- call void @void_byval_args(ptr byval(%struct.Foo) @foo)
- ret void
-}
-
declare void @void_sret_args(ptr sret(%struct.Foo) %f)
define void @test_void_sret_args() {
diff --git a/llvm/test/CodeGen/RISCV/GlobalISel/riscv-unsupported.ll b/llvm/test/CodeGen/RISCV/GlobalISel/riscv-unsupported.ll
new file mode 100644
index 00000000000000..073b5186aad718
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/GlobalISel/riscv-unsupported.ll
@@ -0,0 +1,16 @@
+; RUN: llc -mtriple=riscv32 -verify-machineinstrs -global-isel -global-isel-abort=2 -pass-remarks-missed='gisel*' %s -o - 2>&1 | FileCheck %s -check-prefixes=CHECK
+; RUN: llc -mtriple=riscv64 -verify-machineinstrs -global-isel -global-isel-abort=2 -pass-remarks-missed='gisel*' %s -o - 2>&1 | FileCheck %s -check-prefixes=CHECK
+
+; This file checks that we use the fallback path for things that are known to
+; be unsupported on the RISC-V target. It should progressively shrink in size.
+
+%byval.class = type { i32 }
+
+declare void @test_byval_arg(ptr byval(%byval.class) %x)
+
+define void @test_byval_param(ptr %x) {
+; CHECK: remark: {{.*}} unable to translate instruction: call
+; CHECK-LABEL: warning: Instruction selection used fallback path for test_byval_param
+ call void @test_byval_arg(ptr byval(%byval.class) %x)
+ ret void
+}
|
lenary
approved these changes
Dec 9, 2024
|
It's not clear to me the AArch64 GlobalISel byval handling code is correct anyway: #62138 (and, the same actually applies to SelectionDAG: #62137) I think RISC-V SDAG is doing something better for byval than AArch64 SDAG (making the copies earlier), but I'm not sure anyone is quite getting byval right with GlobalISel |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Our byval call lowering isn't copying the argument. Looks like our SelectionDAG code for byval is different than AArch64 so this may be non-trivial to fix. Reject for now.