Page MenuHomePhabricator

[clang][patch] Add support for option -fextend-arguments={32,64}: widen integer arguments to int64 in unprototyped function calls
ClosedPublic

Authored by mibintc on Apr 30 2021, 8:19 AM.

Details

Summary

The Intel C++ and Fortran compilers support the option -fextend-arguments={32,64}. One of our customers has requested that this support be added clang. This option controls how scalar integral arguments are extended in calls to unprototyped and varargs functions. When we investigated the support for the ICL option, we discovered that "-fextend-arguments=32" had no effect in the generated code, so this patch passes the option into the compiler only for intel64, otherwise the width defaults to 32 bits as usual. This patch is only meaningful for targets that pass int arguments in 64 bits.

For supported targets, signed int values are sign extended to 64 bits in the parameter list, and unsigned int values are zero extended to 64 bits.

@kbsmith1 tells me that this option is primarily useful for porting 32-bit programs to the 64-bit environment, and although that architecture shift happened years ago, the customer is still interested in this support.

It turns out that there is currently similar logic in clang support for the OpenCL option "cl_khr_fp64" which causes certain float arguments, in unprototyped context, to be expanded to float or double.

Will this change be acceptable?

Diff Detail

Unit TestsFailed

TimeTest
170 msx64 windows > LLVM.Other::new-pm-lto-defaults.ll
Script: -- : 'RUN: at line 5'; c:\ws\w16e2-2\llvm-project\premerge-checks\build\bin\opt.exe -disable-verify -verify-cfg-preserved=0 -debug-pass-manager -passes='lto<O1>' -S C:\ws\w16e2-2\llvm-project\premerge-checks\llvm\test\Other\new-pm-lto-defaults.ll 2>&1 | c:\ws\w16e2-2\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w16e2-2\llvm-project\premerge-checks\llvm\test\Other\new-pm-lto-defaults.ll --check-prefix=CHECK-O --check-prefix=CHECK-O1

Event Timeline

mibintc created this revision.Apr 30 2021, 8:19 AM
mibintc requested review of this revision.Apr 30 2021, 8:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 30 2021, 8:19 AM

@erichkeane Can you suggest reviewers for this patch?

Usually, Clang has two flags to control a feature: one to turn it on and the other to turn it off. The last one on the command-line wins.

Accepting both -fextend-arguments=64 and -fextend-arguments=32 would be more consistent with the rest of the command-line options IMO.

mibintc updated this revision to Diff 342478.May 3 2021, 11:30 AM
mibintc retitled this revision from [clang][patch] Add support for option -fextend-arguments-64: widen integer arguments to int64 in unprototyped function calls to [clang][patch] Add support for option -fextend-arguments={32,64}: widen integer arguments to int64 in unprototyped function calls.

Respond to review comments from @jansvoboda11

mibintc edited the summary of this revision. (Show Details)May 3 2021, 11:32 AM

Nice, that looks better. I think we can optimize it a tad by introducing an enum with two cases and store that in LangOptions instead of a 32-bit number.

clang/include/clang/Basic/LangOptions.def
417

Since we're accepting only two values (either 32 or 64), we can define an enum for those values and reduce size of this member from 32 bits to just 1.

clang/include/clang/Driver/Options.td
1456

The MarshallingInfoEnum multiclass can be used instead (docs).

mibintc updated this revision to Diff 342750.May 4 2021, 8:28 AM

Responded to suggestions from @jansvoboda11

Thanks! This LGTM from command-line perspective, but I'll let others judge the functional change.

aaron.ballman added inline comments.
clang/include/clang/Basic/LangOptions.h
262–264

The comments aren't quite right as this matters in functions with prototypes as well (due to variadic arguments). How about "integer arguments are sign or zero extended to 32/64 bits during default argument promotions."?

clang/include/clang/Basic/TargetInfo.h
1414

This should get a comment explaining what it does. The name is a bit generic too (esp given that there's an _ExtInt type that sounds like an "extended int").

Is there anything truly target-specific about the functionality? It seems like this behavior would be plausibly useful regardless of target architecture.

clang/include/clang/Driver/Options.td
1453–1454

It's a bit more conventional to use a trailing whitespace in adjacent string literals.

clang/test/CodeGen/extend-arg-64.c
20

Can you also add a test for K&R C functions? e.g., int knr(); sum = knr(sum, u32, s32, u16, s16, u8, s8);

Also, can you show what happens when passing a long long and an _ExtInt

mibintc added inline comments.May 12 2021, 1:33 PM
clang/test/CodeGen/extend-arg-64.c
20

Yes I can add those tests. BTW the _ExtInt types are not "promotable" therefore the UsualUnaryConversions have no effect.

aaron.ballman added inline comments.May 13 2021, 5:03 AM
clang/test/CodeGen/extend-arg-64.c
20

Yes I can add those tests. BTW the _ExtInt types are not "promotable" therefore the UsualUnaryConversions have no effect.

Thanks! And yeah -- I figured that would be a good test to ensure that this flag doesn't impact types that wouldn't typically go through default argument promotion. Come to think of it, it may be good to have a test for float and double to show that those aren't impacted despite being promotable (with some comments explaining why).

mibintc updated this revision to Diff 345154.May 13 2021, 8:07 AM

I responded to most all @aaron.ballman 's review comments, may need a little more wordsmithing

Thanks! The last remaining question to me is whether this should be a target-specific option or whether it makes sense to allow it as an option for any target.

clang/include/clang/Basic/LangOptions.def
418–419

I missed this one last time.

Thanks! The last remaining question to me is whether this should be a target-specific option or whether it makes sense to allow it as an option for any target.

I thought the patch may be more acceptable to the community if we restricted it.

Thanks! The last remaining question to me is whether this should be a target-specific option or whether it makes sense to allow it as an option for any target.

I thought the patch may be more acceptable to the community if we restricted it.

I think we usually prefer more general solutions to target-specific ones, typically. Given that the functionality is generally useful for easing porting projects from 32- to 64-bits and that the code is simpler without the target-specific bits, my preference is to go with the general solution unless someone else has objections.

Thanks! The last remaining question to me is whether this should be a target-specific option or whether it makes sense to allow it as an option for any target.

I thought the patch may be more acceptable to the community if we restricted it.

I think we usually prefer more general solutions to target-specific ones, typically. Given that the functionality is generally useful for easing porting projects from 32- to 64-bits and that the code is simpler without the target-specific bits, my preference is to go with the general solution unless someone else has objections.

Actually I thought about it some more and I don't know, generally, in what condition to enable the option. For example, on x86 we don't want any transformation made, and on intel64 we do want the implicit cast inserted.

Thanks! The last remaining question to me is whether this should be a target-specific option or whether it makes sense to allow it as an option for any target.

I thought the patch may be more acceptable to the community if we restricted it.

I think we usually prefer more general solutions to target-specific ones, typically. Given that the functionality is generally useful for easing porting projects from 32- to 64-bits and that the code is simpler without the target-specific bits, my preference is to go with the general solution unless someone else has objections.

Actually I thought about it some more and I don't know, generally, in what condition to enable the option. For example, on x86 we don't want any transformation made, and on intel64 we do want the implicit cast inserted.

Okay, it sounds like it's not the general win I was thinking it might be. I'm fine making this target-specific for now, and we can extend it in the future to other targets.

mibintc updated this revision to Diff 345273.May 13 2021, 1:50 PM

Corrected the failing lit test and applied the clang-format patch

aaron.ballman accepted this revision.May 14 2021, 9:37 AM

The changes LGTM, but you should wait a bit before landing in case @rsmith or one of the other reviewers has concerns.

This revision is now accepted and ready to land.May 14 2021, 9:37 AM
This revision was landed with ongoing or failed builds.May 19 2021, 8:00 AM
This revision was automatically updated to reflect the committed changes.