[GISel][CallLowering] Set byval flag for inalloca/preallocated args#200600
[GISel][CallLowering] Set byval flag for inalloca/preallocated args#200600lonemeow wants to merge 2 commits into
Conversation
addFlagsFromAttrSet set the InAlloca/Preallocated flags but not ByVal, so the later setByValSize() call in setArgFlags tripped its `assert(isByVal() && !isByRef())` and crashed when lowering inalloca or preallocated arguments (e.g. 32-bit x86 calls that pass non-trivially copyable objects by value). Also set the ByVal flag for these attributes, matching SelectionDAGISel::LowerArguments, so the allocated byte count reaches the calling-convention lowering callbacks.
|
Hello @lonemeow 👋 Thank you for submitting a Pull Request (PR) to the LLVM Project. Since this is your first PR, here are a few useful links covering our main contribution policies and review practices.
Please reply to this message to confirm that you have read these policies, especially the LLVM AI Tool Use Policy, and that any AI tool usage has been noted in the PR description. Frequently asked questionsHow do I add reviewers? This PR will be automatically labeled, and the relevant teams will be notified. For some parts of the project, reviewers may also be added automatically. You can also add reviewers manually using the Reviewers section on this page. If you cannot use that section, it is probably because you do not have write permissions for the repository. In that case, you can request a review by tagging reviewers in a comment using What if there are no comments? If you have not received any comments on your PR after a week, you can request a review by pinging the PR with a comment such as “Ping”. The common courtesy ping rate is once a week. Please remember that you are asking for volunteer time from other developers. Are any special GitHub settings required to contribute to LLVM? We only require contributors to have a public email address associated with their GitHub commits, see this section of LLVM Developer Policy for details. If you have questions, feel free to leave a comment on this PR, or ask on LLVM Discord or LLVM Discourse. Thank you, |
|
@llvm/pr-subscribers-llvm-globalisel @llvm/pr-subscribers-backend-x86 Author: Ilpo Ruotsalainen (lonemeow) ChangesGlobalISel asserts when lowering a function with an > TargetCallingConv.h:183: void llvm::ISD::ArgFlagsTy::setByValSize(unsigned): Assertion `isByVal() && !isByRef()' failed. https://godbolt.org/z/jWr4enjaj
Full diff: https://github.com/llvm/llvm-project/pull/200600.diff 2 Files Affected:
diff --git a/llvm/lib/CodeGen/GlobalISel/CallLowering.cpp b/llvm/lib/CodeGen/GlobalISel/CallLowering.cpp
index b0bf4b684bf6d..224e2e361bb9a 100644
--- a/llvm/lib/CodeGen/GlobalISel/CallLowering.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/CallLowering.cpp
@@ -51,10 +51,17 @@ static void addFlagsFromAttrSet(ISD::ArgFlagsTy &Flags, AttributeSet Attrs) {
Flags.setByVal();
if (Attrs.hasAttribute(Attribute::ByRef))
Flags.setByRef();
- if (Attrs.hasAttribute(Attribute::Preallocated))
+ if (Attrs.hasAttribute(Attribute::Preallocated)) {
Flags.setPreallocated();
- if (Attrs.hasAttribute(Attribute::InAlloca))
+ // Set the byval flag so CC lowering callbacks that don't know about
+ // preallocated still see the correct allocated byte count.
+ Flags.setByVal();
+ }
+ if (Attrs.hasAttribute(Attribute::InAlloca)) {
Flags.setInAlloca();
+ // Likewise set byval for inalloca; see the preallocated case above.
+ Flags.setByVal();
+ }
if (Attrs.hasAttribute(Attribute::Returned))
Flags.setReturned();
if (Attrs.hasAttribute(Attribute::SwiftSelf))
diff --git a/llvm/test/CodeGen/X86/GlobalISel/calllowering-inalloca.ll b/llvm/test/CodeGen/X86/GlobalISel/calllowering-inalloca.ll
new file mode 100644
index 0000000000000..9fac886527c39
--- /dev/null
+++ b/llvm/test/CodeGen/X86/GlobalISel/calllowering-inalloca.ll
@@ -0,0 +1,23 @@
+; NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+; RUN: llc -mtriple=i686-pc-win32 -global-isel -stop-after=irtranslator -verify-machineinstrs %s -o - | FileCheck %s
+
+; inalloca and preallocated formal arguments used to crash CallLowering with
+; an isByVal() assertion in setByValSize; check they translate instead.
+
+define void @inalloca_arg(ptr inalloca(i32) %p) {
+ ; CHECK-LABEL: name: inalloca_arg
+ ; CHECK: bb.1 (%ir-block.0):
+ ; CHECK-NEXT: [[FRAME_INDEX:%[0-9]+]]:_(p0) = G_FRAME_INDEX %fixed-stack.0
+ ; CHECK-NEXT: [[COPY:%[0-9]+]]:_(p0) = COPY [[FRAME_INDEX]](p0)
+ ; CHECK-NEXT: RET 0
+ ret void
+}
+
+define void @preallocated_arg(ptr preallocated(i32) %p) {
+ ; CHECK-LABEL: name: preallocated_arg
+ ; CHECK: bb.1 (%ir-block.0):
+ ; CHECK-NEXT: [[FRAME_INDEX:%[0-9]+]]:_(p0) = G_FRAME_INDEX %fixed-stack.0
+ ; CHECK-NEXT: [[COPY:%[0-9]+]]:_(p0) = COPY [[FRAME_INDEX]](p0)
+ ; CHECK-NEXT: RET 0
+ ret void
+}
|
| @@ -0,0 +1,23 @@ | |||
| ; NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py | |||
| ; RUN: llc -mtriple=i686-pc-win32 -global-isel -stop-after=irtranslator -verify-machineinstrs %s -o - | FileCheck %s | |||
There was a problem hiding this comment.
| ; RUN: llc -mtriple=i686-pc-win32 -global-isel -stop-after=irtranslator -verify-machineinstrs %s -o - | FileCheck %s | |
| ; RUN: llc -global-isel -mtriple=i686-pc-win32 -stop-after=irtranslator %s -o - | FileCheck %s |
| // Set the byval flag so CC lowering callbacks that don't know about | ||
| // preallocated still see the correct allocated byte count. | ||
| Flags.setByVal(); |
There was a problem hiding this comment.
Does SelectionDAG do this same hack? If not, it shouldn't be done here
There was a problem hiding this comment.
Can you copy the full comment from where the DAG does this
There was a problem hiding this comment.
Does SelectionDAG do this same hack? If not, it shouldn't be done here
Use the verbatim CCAssignFn comment from SelectionDAGBuilder for the inalloca/preallocated byval flag, and reorder the llc test invocation flags as suggested.
2 participants
GlobalISel asserts when lowering a function with an
inallocaorpreallocatedargument:https://godbolt.org/z/jWr4enjaj
addFlagsFromAttrSet()sets theInAlloca/Preallocatedflags but neverByVal.setArgFlags()then callsFlags.setByValSize()for any non-ByRefindirect argument which assertsisByVal().SelectionDAGISel::LowerArguments()avoids this by deliberately also setting theByValflag forinalloca/preallocated, this change makes the GlobalISel side match that behavior.