This is an archive of the discontinued LLVM Phabricator instance.

Differential D90230

[SVE] Add fatal error for unnamed SVE variadic arguments
ClosedPublic

Authored by david-arm on Oct 27 2020, 7:19 AM.

Details

Reviewers
sdesmalen
c-rhodes
rengolin
efriedma
Commits
rGcea69fa4dcc4: [SVE] Add fatal error for unnamed SVE variadic arguments
Summary

We don't currently support passing unnamed variadic SVE arguments
so I've added a fatal error if we hit such cases to prevent any
silent ABI issues in future.

Diff Detail

Unit TestsFailed

TimeTest
420 mslinux > HWAddressSanitizer-x86_64.TestCases::sizes.cpp
100 mslinux > MLIR.Target::llvmir.mlir
30 mswindows > Clang-Unit.Format/_/FormatTests_exe::TokenAnnotatorTest.UnderstandsUsesOfStarAndAmpInMacroDefinition
110 mswindows > MLIR.Target::llvmir.mlir
470 mswindows > lld.ELF/invalid::symtab-sh-info.s

Event Timeline

david-arm created this revision.Oct 27 2020, 7:19 AM
Herald added a reviewer: rengolin. · View Herald TranscriptOct 27 2020, 7:19 AM
Herald added a reviewer: efriedma. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: llvm-commits, psnobl, hiraditya, tschuett. · View Herald Transcript
david-arm requested review of this revision.Oct 27 2020, 7:19 AM
Harbormaster completed remote builds in B76545: Diff 300981.Oct 27 2020, 8:02 AM
sdesmalen added inline comments.Oct 27 2020, 9:33 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
4802

Does this also report a fatal error if ArgVT is a named argument?

i.e. declare i32 @sve_printf(i8*, <vscale x 4 x i32> %v, ...) and then calling with:

call i32 (i8*, <vscale x 4 x i32>, ...) @sve_printf(i8* %f, <vscale x 4 x i32> %x)
david-arm updated this revision to Diff 301057.Oct 27 2020, 10:51 AM
david-arm marked an inline comment as done.
efriedma added a comment.Oct 27 2020, 10:52 AM

We should have corresponding handling for va_arg. (clang doesn't produce va_arg instructions, but other frontends do.)

david-arm updated this revision to Diff 301567.Oct 29 2020, 4:19 AM
  • Added fatal errors on the callee side when one of the variadic arguments is a scalable vector.
efriedma accepted this revision.Oct 29 2020, 11:40 AM

LGTM

This revision is now accepted and ready to land.Oct 29 2020, 11:40 AM
sdesmalen accepted this revision.Oct 30 2020, 4:01 AM
Closed by commit rGcea69fa4dcc4: [SVE] Add fatal error for unnamed SVE variadic arguments (authored by david-arm). · Explain WhyOct 30 2020, 6:36 AM
This revision was automatically updated to reflect the committed changes.
david-arm added a commit: rGcea69fa4dcc4: [SVE] Add fatal error for unnamed SVE variadic arguments.
Herald added a project: Restricted Project. · View Herald TranscriptOct 30 2020, 6:36 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Revision Contents

PathSize
llvm/
lib/
Target/
AArch64/
4 lines
test/
CodeGen/
AArch64/
12 lines

Diff 300981

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

  • This file is larger than 256 KB, so syntax highlighting is disabled by default.
Show First 20 Lines • Show All 4,792 Lines • ▼ Show 20 LinesAArch64TargetLowering::LowerCall(CallLoweringInfo &CLI,
if (IsVarArg) { if (IsVarArg) {
// Handle fixed and variable vector arguments differently. // Handle fixed and variable vector arguments differently.
// Variable vector arguments always go into memory. // Variable vector arguments always go into memory.
unsigned NumArgs = Outs.size(); unsigned NumArgs = Outs.size();
for (unsigned i = 0; i != NumArgs; ++i) { for (unsigned i = 0; i != NumArgs; ++i) {
MVT ArgVT = Outs[i].VT; MVT ArgVT = Outs[i].VT;
if (ArgVT.isScalableVector())
report_fatal_error("Passing SVE types to variadic functions is "
sdesmalenUnsubmitted
Done
ReplyInline Actions

Does this also report a fatal error if ArgVT is a named argument?

i.e. declare i32 @sve_printf(i8*, <vscale x 4 x i32> %v, ...) and then calling with:

call i32 (i8*, <vscale x 4 x i32>, ...) @sve_printf(i8* %f, <vscale x 4 x i32> %x)
sdesmalen: Does this also report a fatal error if `ArgVT` is a named argument? i.e. `declare i32…
"currently not supported");
ISD::ArgFlagsTy ArgFlags = Outs[i].Flags; ISD::ArgFlagsTy ArgFlags = Outs[i].Flags;
CCAssignFn *AssignFn = CCAssignFnForCall(CallConv, CCAssignFn *AssignFn = CCAssignFnForCall(CallConv,
/*IsVarArg=*/ !Outs[i].IsFixed); /*IsVarArg=*/ !Outs[i].IsFixed);
bool Res = AssignFn(i, ArgVT, ArgVT, CCValAssign::Full, ArgFlags, CCInfo); bool Res = AssignFn(i, ArgVT, ArgVT, CCValAssign::Full, ArgFlags, CCInfo);
assert(!Res && "Call operand has unhandled type"); assert(!Res && "Call operand has unhandled type");
(void)Res; (void)Res;
} }
} else { } else {
▲ Show 20 Lines • Show All 11,528 Lines • Show Last 20 Lines

llvm/test/CodeGen/AArch64/sve-varargs.ll

  • This file was added.
; RUN: not --crash llc -mtriple aarch64-linux-gnu -mattr=+sve <%s 2>&1 | FileCheck %s
declare i32 @sve_printf(i8*, ...)
@.str_1 = internal constant [6 x i8] c"boo!\0A\00"
; CHECK: Passing SVE types to variadic functions is currently not supported
define void @foo(<vscale x 4 x i32> %x) {
%f = getelementptr [6 x i8], [6 x i8]* @.str_1, i64 0, i64 0
call i32 (i8*, ...) @sve_printf(i8* %f, <vscale x 4 x i32> %x)
ret void
}