[FastISel] Lower call instruction with illegal type returned#180322
[FastISel] Lower call instruction with illegal type returned#180322
Conversation
|
@llvm/pr-subscribers-llvm-selectiondag @llvm/pr-subscribers-backend-x86 Author: Luo Yuanke (LuoYuanke) ChangesFull diff: https://github.com/llvm/llvm-project/pull/180322.diff 2 Files Affected:
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
index 27131e14141cc..3010c4e6430f2 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -2019,8 +2019,14 @@ SDValue SelectionDAGBuilder::getValueImpl(const Value *V) {
std::optional<CallingConv::ID> CallConv;
auto *CB = dyn_cast<CallBase>(Inst);
- if (CB && !CB->isInlineAsm())
- CallConv = CB->getCallingConv();
+
+ if (CB && !CB->isInlineAsm()) {
+ if (Inst->getType()->isFirstClassType()) {
+ EVT VT = TLI.getValueType(DAG.getDataLayout(), Inst->getType());
+ if (!TLI.isTypeLegal(VT))
+ CallConv = CB->getCallingConv();
+ }
+ }
RegsForValue RFV(*DAG.getContext(), TLI, DAG.getDataLayout(), InReg,
Inst->getType(), CallConv);
diff --git a/llvm/test/CodeGen/X86/fast-isel-v4i1.ll b/llvm/test/CodeGen/X86/fast-isel-v4i1.ll
new file mode 100644
index 0000000000000..2e9a26a93d507
--- /dev/null
+++ b/llvm/test/CodeGen/X86/fast-isel-v4i1.ll
@@ -0,0 +1,27 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 6
+; RUN: llc --fast-isel < %s -mtriple=x86_64-unknown-unknown | FileCheck %s
+
+define fastcc i16 @test() #0 nounwind {
+; CHECK-LABEL: test:
+; CHECK: # %bb.0: # %entry
+; CHECK-NEXT: pushq %rax
+; CHECK-NEXT: xorl %eax, %eax
+; CHECK-NEXT: vpxor %xmm0, %xmm0, %xmm0
+; CHECK-NEXT: xorl %edi, %edi
+; CHECK-NEXT: callq *%rax
+; CHECK-NEXT: vpslld $31, %xmm0, %xmm0
+; CHECK-NEXT: xorl %eax, %eax
+; CHECK-NEXT: xorl %edi, %edi
+; CHECK-NEXT: vpmovd2m %xmm0, %k0
+; CHECK-NEXT: vpxor %xmm0, %xmm0, %xmm0
+; CHECK-NEXT: vpmovm2d %k0, %xmm1
+; CHECK-NEXT: callq *%rax
+; CHECK-NEXT: popq %rcx
+; CHECK-NEXT: retq
+entry:
+ %0 = call fastcc <4 x i1> null(ptr null, <4 x i1> zeroinitializer)
+ %1 = call fastcc i16 null(ptr null, <4 x i1> zeroinitializer, <4 x i1> %0)
+ ret i16 %1
+}
+
+attributes #0 = { "target-cpu"="znver5" }
|
There was a problem hiding this comment.
Avoid UB in tests, and use named values
There was a problem hiding this comment.
The type legality should not be a factor here
1a67704 to
447762b
Compare
|
✅ With the latest revision this PR passed the undef deprecator. |
447762b to
eab7476
Compare
When lowering the call instruction with illegal type returned, we should bail out and transfer the lowering to DAG. Otherwise the return value is not promoted to proper type, but DAG would assume it has been promoted.
eab7476 to
b25aea6
Compare
llvm/lib/Target/X86/X86FastISel.cpp
Outdated
There was a problem hiding this comment.
This logic kind of makes sense, but this is too narrow, and I would expect would already logically happen. Why wouldn't the same apply for the input arguments? I thought fast isel already gave up on illegally typed operations, so why does this need a new special case?
There was a problem hiding this comment.
The input arguments are checked at https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/X86/X86FastISel.cpp#L3306
There was a problem hiding this comment.
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/Mips/MipsFastISel.cpp#L1528 is how MIPS handle return type.
There was a problem hiding this comment.
I wouldn't use mips as a reference on how to do anything
|
Is this fixing #179100? If so, please reference it in the PR description. |
Done |
Co-authored-by: Craig Topper <craig.topper@sifive.com>
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
I'd prefer this test file was called pr179100.ll for better reference
There was a problem hiding this comment.
Probably should be processing the processed return type, instead of this. AArch64 example:
There was a problem hiding this comment.
I think CLI.OutVals is not the returned values. X86 has code to check it at https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/X86/X86FastISel.cpp#L3306
|
Friendly ping on reviews since the bug this is fixing will be a problem for Zig's LLVM 22 upgrade. |
|
ping |
There was a problem hiding this comment.
These are redundant but functional correct. I think it's not a big problem for fast isel.
|
I think the required change from @arsenm has been addressed. I'll land the patch if there is no objection. |
|
For the record, this causes an |
Switching from fast ISel to DAG ISel would spent more compile-time on ISel. The i1 return type is promoted to i8 which is the same in ISel promotion. The bf16 return type is bf16, while it is promoted to f32 in ISel. Maybe we can check if the promoted type is the same to abi return type before switching form fast ISel to DAG ISel. |
…0322) Fix issue llvm#179100 When lowering the call instruction with illegal type returned, we should bail out and transfer the lowering to DAG. Otherwise the return value is not promoted to proper type, but DAG would assume it has been promoted. --------- Co-authored-by: Yuanke Luo <ykluo@birentech.com> (cherry picked from commit 41ef3d0)
|
I create a PR (#186723) to improve the compile-time. |
…lvm#180322)" This reverts commit 4dfb4b6.
…lvm#180322)" This reverts commit 4dfb4b6. This causes a large compile-time regression for debug builds.
Fix issue #179100
When lowering the call instruction with illegal type returned, we should
bail out and transfer the lowering to DAG. Otherwise the return value is
not promoted to proper type, but DAG would assume it has been promoted.