This is an archive of the discontinued LLVM Phabricator instance.

target ABI: improve call parameters extensions handling
Needs ReviewPublic

Authored by jonpa on Nov 1 2021, 11:12 AM.

Details

Summary

As discussed on llvm-dev (https://lists.llvm.org/pipermail/llvm-dev/2021-October/153080.html), there has been a recurring problem of missing extension attributes on call arguments.

This patch tries to improve on the situation by intorducing a 'NoExt' attribute that is (at least in theory, at the moment) mandatory for a struct passed in an integer register. By demanding that a (narrow) integer argument has one of the sign/zero/no extension attributes, cases where this has been forgotten in the FE can be isolated.

Changes:

  • Clang FE:
    • A new bit ZeroExt in ABIArgInfo. If Kind is Extend, it is SignExt or ZeroExt, or else NoExt is implied.
    • A new method ABIArgInfo::getNoExtend(), used by SystemZABIInfo::classifyArgumentType() for a struct-in-reg argument. (CGCall.cpp:CodeGenTypes::arrangeLLVMFunctionInfo() calls SystemZABIInfo::computeInfo() and the argument types are classified. How about Swift (or SPIR_KERNEL) CCs: should they also have narrow integer arguments extended on SystemZ?)
  • IR: new attribute NoExt, handled by parser, emitter etc. Adding one bit IsNoExt ArgFlagsTy grew it past the 3-byte size, so the static_assert was incremented to match the new size of 4 bytes. Any problem with this?
  • SystemZ backend:
    • Default sign extension of <=32 bit arguments added, which can be disabled with '-no-arg-exts'.
    • New option -verify-int-arg-exts (default off), that when passed checks that all <=32 bit integer arguments have a valid extension attribute. Verifying extension of return values and outgoing call arguments: Incoming function parameters and call results could also be checked, but they are not needed for correctness, but only to let the backend assume they are sign-extended properly.
    • A lot of tests updated, mostly with -no-arg-exts, which makes sense in cases like testing for a three-address instruction selection. It seems reasonable to simply disable this default sign-extension during testing, but in a way it may in the future be better to instead update the tests. There are more tests that can be updated - I have only changed the ones that seemed trivial so far. (args-04.ll: This is testing incoming arguments, but it is returning them without extension. This is a little weird as structs are never returned in a reg..?)
  • Documentation: Explanation of the NoExt attribute and why it is mandatory.

The comment for getTypeForExtReturn() may be related to this issue..? "// Return the type that should be used to zero or sign extend a zeroext/signext integer return value. FIXME: Some C calling conventions require the return type to be promoted, but this is not true all the time, e.g. i1/i8/i16 on x86/x86_64. It is also not necessary for non-C calling conventions. The frontend should handle this and include all of the necessary information."

So far, this is a work in progress so feel free to make suggestions...

Diff Detail

Event Timeline

jonpa created this revision.Nov 1 2021, 11:12 AM
jonpa requested review of this revision.Nov 1 2021, 11:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 1 2021, 11:12 AM
rnk added a subscriber: aeubanks.Nov 1 2021, 3:17 PM

Seems reasonable to me. +@aeubanks, can you review this? I think you ran into these issues with SystemZ extension attributes when you made that change to only look at call site ABI attributes.

llvm/include/llvm/IR/Attributes.td
139

This is not SystemZ-specific, so instead of "struct in reg" I suggest "high bits are undefined". That should be accurate when this attribute is used on any target.

ormris removed a subscriber: ormris.Jan 24 2022, 11:30 AM

I ran this on SPEC (with -mllvm -verify-int-arg-exts), and believe I found two things that need to be fixed:

Argument promotion: https://github.com/llvm/llvm-project/issues/54530
InstCombine libcalls: https://github.com/llvm/llvm-project/issues/54532

Herald added a project: Restricted Project. · View Herald TranscriptMar 24 2022, 10:10 AM
jonpa updated this revision to Diff 418399.Mar 26 2022, 9:01 AM
jonpa marked an inline comment as done.

Patch updated:

  • only do a verification / default sign extension when an externally visible function is involved. IR producers such as Argument Promotion is free to change the ABI and pass a non-extended argument as long as it is only in local scope.
  • Allow NoExt attribute also on returned values.
  • Tests updated so that no tests are changed, but instead the -no-arg-exts CL option is passed, which makes the default sign extension to be skipped.

I found that with this patch, which enables the default sign extension by default, on SPEC I mainly see a lot of lhi -> lghi changes, with not many more instructions overall:

lhi            :               219521               218299    -1222
lghi           :               445907               447129    +1222
llgfr          :                89262                89289      +27
chi            :                53164                53180      +16
lbh            :                 1141                 1125      -16
lb             :                19083                19099      +16
locfhrl        :                   27                   11      -16
locrl          :                 2922                 2938      +16
llcr           :                 5076                 5092      +16
lr             :                63302                63287      -15
risblg         :                17057                17042      -15
cih            :                 6728                 6715      -13
lgfr           :                89323                89329       +6

...
OPCDIFFS: +22

jonpa updated this revision to Diff 427009.May 4 2022, 8:21 AM
jonpa added a reviewer: efriedma.

minor fixing after rebase.

After I committed the proper handling of libcalls emission I have now built SPEC17, the test-suite tests and also bootstrapped LLVM with this patch applied and verified it all without any more errors. So right now those attributes look "green" on SystemZ with the Clang front end and I think it would be useful to continue with this patch. There are two things here of interest:

  • verifying argument attributes.

Should this check be moved from the SystemZ backend out into common code and use TLI.getExtAttrForI32Param() to enable selectively? That function is for library calls, but I think they should apply to any call?

  • adding a default extension in case of a missing attribute which is better than nothing.

Maybe this should also be moved out in the same way and enabled with some similar hook? I think for SystemZ we will use a sign extension and think that is probably what everyone wants (the most likely to help).

Ping!

Anyone else thinks this verification / default extension should be placed in common code instead of in just the SystemZ backend?

Not sure how much I like the rule about "externally visible functions"... I mean, I guess restricting the checking to external functions avoids triggering on a bunch of cases that would be difficult to handle, but it doesn't really seem self-consistent.

How about this: we introduce NoExt, but in LangRef we just say "If an integer argument to a function is not marked signext/zeroext/noext, the kind of extension is used is target-specific. Some targets may generate less efficient code if the kind of extension is not explicitly specified." This is basically what you're doing now: follow the attribute, and sign-extend in the caller if there's no attribute. But there isn't an implication that generating calls without any extension attributes is invalid on all targets.

For the lint in the backend, instead of checking whether the callee is an external symbol, maybe check whether the calling convention is "C" (as opposed to "fast").

jonpa added a comment.May 23 2022, 5:43 AM

Not sure how much I like the rule about "externally visible functions"... I mean, I guess restricting the checking to external functions avoids triggering on a bunch of cases that would be difficult to handle, but it doesn't really seem self-consistent.

Well, if we decide to in llvm *not* do the optimization of removing an extension of an incoming argument (which the ABI says is legal), our only concern is when calling or returning a value from an externally visible function. I think this is a legacy issue and not important for performance, so acting on the attribute when compiling is not something we plan to do (at least on SystemZ).

How about this: we introduce NoExt, but in LangRef we just say "If an integer argument to a function is not marked signext/zeroext/noext, the kind of extension is used is target-specific. Some targets may generate less efficient code if the kind of extension is not explicitly specified." This is basically what you're doing now: follow the attribute, and sign-extend in the caller if there's no attribute. But there isn't an implication that generating calls without any extension attributes is invalid on all targets.

I consider the "default extension" to be related but not really the same idea: it merely serves as a "better than nothing" solution, but I think that may be removed in the future, and the verification instead is on by default. Actually, this could be done for SystemZ now at least as things look "green" now. Note that this default extension could be wrong (use SExt instead of ZExt on a very big unsigned value).

How about:
"If an integer argument to a function is not marked signext/zeroext/noext, the kind of extension used is target-specific. Some targets depend for correctness the kind of extension to be explicitly specified."

For the lint in the backend, instead of checking whether the callee is an external symbol, maybe check whether the calling convention is "C" (as opposed to "fast").

Do all externally visible functions always have the "C" calling convention?

Should the VerifyIntegerArg() and CheckNarrowIntegerArgs() remain in SystemZ or should I move them somewhere and let any target use them as desired? I think the default extension could be done if desired entirely in the target backend, or?

How about:
"If an integer argument to a function is not marked signext/zeroext/noext, the kind of extension used is target-specific. Some targets depend for correctness the kind of extension to be explicitly specified."

I guess that's sufficient.

For the lint in the backend, instead of checking whether the callee is an external symbol, maybe check whether the calling convention is "C" (as opposed to "fast").

Do all externally visible functions always have the "C" calling convention?

Unless you're using some attribute to modify the calling convention, everything in clang defaults to "C". Some targets, like x86 Windows, frequently use alternate calling conventions, but I assume SystemZ doesn't.

Should the VerifyIntegerArg() and CheckNarrowIntegerArgs() remain in SystemZ or should I move them somewhere and let any target use them as desired? I think the default extension could be done if desired entirely in the target backend, or?

I think I'm okay leaving it in target-specific code; unifying anything related to calls is hard.

jonpa added a comment.Sep 13 2022, 4:20 AM

Unless you're using some attribute to modify the calling convention, everything in clang defaults to "C". Some targets, like x86 Windows, frequently use alternate calling conventions, but I assume SystemZ doesn't.

I still feel more comfortable with the "external / internal" heuristic, as the whole idea is that we actually are using a trick by assuming we will be safe in clang by never acting on this optimization. For me this attribute is more readable as it reflects the idea directly, compared to checking for "C". I also feel that it is very precise and can therefore never go wrong.

I think I'm okay leaving it in target-specific code; unifying anything related to calls is hard.

Ok, makes sense.

SPEC builds fine now after the handling of library calls arguments extensions we did earlier, but now have encountered problems when building the llvm tests: AddressSanitizer seems to be dropping these attributes:

Compiling gtest-all (reduced):

extern "C" void *memset(void *, int, long);
memset(&hints, 0, sizeof(hints));    

=>

declare ptr @memset(ptr noundef, i32 noundef signext, i64 noundef)
%call = call ptr @memset(ptr noundef %hints, i32 noundef signext 0, i64 noundef 16)

=>  InstCombine

declare void @llvm.memset.p0.i64(ptr nocapture writeonly, i8, i64, i1 immarg)
call void @llvm.memset.p0.i64(ptr noundef nonnull align 8 dereferenceable(16) %hints, i8 noundef signext 0, i64 nound
ef 16, i1 false)

=>  AddressSanitizer

declare ptr @__asan_memset(ptr, i32, i64)
%24 = call ptr @__asan_memset(ptr %12, i32 0, i64 16)

This last step is replacing an intrinsic with a call to a global function. I believe there may be some work in progress on how these attributes are supposed to be eventually managed (only in declaration, only on call instruction, either, or both), and would like to first ask how I should go about this: InstCombiner is transferring the attribute when it's building the replacing call, and it seems that AddressSanitizer should do the same. Or is there another preferred approach?

"signext" doesn't really make sense on llvm.memset: it's not a call. It's an intrinsic. It might be lowered to a call, but that's not really relevant: even if we do generate a call, there's no reason to expect that call's calling convention is the same as memset's calling convention. instcombine transferring it is just an accident.

The ASan instrumentation pass should be able to figure out the correct attributes for any functions it calls from first principles (possibly using TargetLibraryInfo::getExtAttrForI32Param(), like other instrumentation passes).

jonpa added a comment.Sep 15 2022, 8:48 AM

"signext" doesn't really make sense on llvm.memset: it's not a call. It's an intrinsic. It might be lowered to a call, but that's not really relevant: even if we do generate a call, there's no reason to expect that call's calling convention is the same as memset's calling convention. instcombine transferring it is just an accident.

The ASan instrumentation pass should be able to figure out the correct attributes for any functions it calls from first principles (possibly using TargetLibraryInfo::getExtAttrForI32Param(), like other instrumentation passes).

ok - started working on that here: https://reviews.llvm.org/D133949