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

[mibintc]

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?

[mibintc]

@erichkeane Can you suggest reviewers for this patch?

[jansvoboda11]

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]

Respond to review comments from @jansvoboda11

[jansvoboda11]

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

418

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

1459

The MarshallingInfoEnum multiclass can be used instead (docs).

[mibintc]

Responded to suggestions from @jansvoboda11

[jansvoboda11]

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

[aaron.ballman]

clang/include/clang/Basic/LangOptions.h

271–273

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

1418

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

1456–1457

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]

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]

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]

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

[aaron.ballman]

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

419–420

I missed this one last time.

[mibintc]

In D101640#2757039, @aaron.ballman wrote:

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.

[aaron.ballman]

In D101640#2757178, @mibintc wrote:

In D101640#2757039, @aaron.ballman wrote:

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.

[mibintc]

In D101640#2757316, @aaron.ballman wrote:

In D101640#2757178, @mibintc wrote:

In D101640#2757039, @aaron.ballman wrote:

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.

[aaron.ballman]

In D101640#2757815, @mibintc wrote:

In D101640#2757316, @aaron.ballman wrote:

In D101640#2757178, @mibintc wrote:

In D101640#2757039, @aaron.ballman wrote:

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]

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

[aaron.ballman]

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

clang/include/clang/Basic/LangOptions.h

public:
 
   enum class ThreadModelKind {
     /// POSIX Threads.
     POSIX,
     /// Single Threaded Environment.
     Single
   };
 
+  enum class ExtendArgsKind {
+    /// Integer arguments are sign or zero extended to 32/64 bits
+    /// during default argument promotions.
+    ExtendTo32,
+    ExtendTo64
+  };
+
 public:
   /// The used language standard.
   LangStandard::Kind LangStd;
 
   /// Set of enabled sanitizers.
   SanitizerSet Sanitize;
 
   /// Paths to files specifying which objects

clang/include/clang/Basic/LangOptions.def

 
 LANGOPT(SpeculativeLoadHardening, 1, 0, "Speculative load hardening enabled")
 
 LANGOPT(RelativeCXXABIVTables, 1, 0,
         "Use an ABI-incompatible v-table layout that uses relative references")
 
 LANGOPT(ArmSveVectorBits, 32, 0, "SVE vector size in bits")
 
+ENUM_LANGOPT(ExtendIntArgs, ExtendArgsKind, 1, ExtendArgsKind::ExtendTo32, 
+             "Controls how scalar integer arguments are extended in calls "
+             "to unprototyped and varargs functions")
+
 #undef LANGOPT
 #undef COMPATIBLE_LANGOPT
 #undef BENIGN_LANGOPT
 #undef ENUM_LANGOPT
 #undef COMPATIBLE_ENUM_LANGOPT
 #undef BENIGN_ENUM_LANGOPT
 #undef VALUE_LANGOPT
 #undef COMPATIBLE_VALUE_LANGOPT
 #undef BENIGN_VALUE_LANGOPT

clang/include/clang/Basic/TargetInfo.h

 
   /// Retrieve the minimum desired version of the platform, to
   /// which the program should be compiled.
   VersionTuple getPlatformMinVersion() const { return PlatformMinVersion; }
 
   bool isBigEndian() const { return BigEndian; }
   bool isLittleEndian() const { return !BigEndian; }
 
+  /// Whether the option -fextend-arguments={32,64} is supported on the target.
+  virtual bool supportsExtendIntArgs() const { return false; }
+
   /// Gets the default calling convention for the given target and
   /// declaration context.
   virtual CallingConv getDefaultCallingConv() const {
     // Not all targets will specify an explicit calling convention that we can
     // express.  This will always do the right thing, even though it's not
     // an explicit calling convention.
     return CC_C;
   }

clang/include/clang/Driver/Options.td

def menable_unsafe_fp_math : Flag<["-"], "menable-unsafe-fp-math">, Flags<[CC1Option]>,
   HelpText<"Allow unsafe floating-point math optimizations which may decrease precision">,
   MarshallingInfoFlag<LangOpts<"UnsafeFPMath">>,
   ImpliedByAnyOf<[cl_unsafe_math_optimizations.KeyPath, ffast_math.KeyPath]>;
 defm math_errno : BoolFOption<"math-errno",
   LangOpts<"MathErrno">, DefaultFalse,
   PosFlag<SetTrue, [CC1Option], "Require math functions to indicate errors by setting errno">,
   NegFlag<SetFalse>>,
   ShouldParseIf<!strconcat("!", open_cl.KeyPath)>;
+def fextend_args_EQ : Joined<["-"], "fextend-arguments=">, Group<f_Group>,
+  Flags<[CC1Option, NoArgumentUnused]>,
+  HelpText<"Controls how scalar integer arguments are extended in calls "
+           "to unprototyped and varargs functions">,
+  Values<"32,64">,
+  NormalizedValues<["ExtendTo32", "ExtendTo64"]>,
+  NormalizedValuesScope<"LangOptions::ExtendArgsKind">,
+  MarshallingInfoEnum<LangOpts<"ExtendIntArgs">,"ExtendTo32">;
 def fbracket_depth_EQ : Joined<["-"], "fbracket-depth=">, Group<f_Group>, Flags<[CoreOption]>;
 def fsignaling_math : Flag<["-"], "fsignaling-math">, Group<f_Group>;
 def fno_signaling_math : Flag<["-"], "fno-signaling-math">, Group<f_Group>;
 defm jump_tables : BoolFOption<"jump-tables",
   CodeGenOpts<"NoUseJumpTables">, DefaultFalse,
   NegFlag<SetTrue, [CC1Option], "Do not use">, PosFlag<SetFalse, [], "Use">,
   BothFlags<[], " jump tables for lowering switches">>;
 defm force_enable_int128 : BoolFOption<"force-enable-int128",

clang/lib/Basic/Targets/X86.h

bool setCPU(const std::string &Name) override {
     CPU = llvm::X86::parseArchX86(Name, Only64Bit);
     return CPU != llvm::X86::CK_None;
   }
 
   unsigned multiVersionSortPriority(StringRef Name) const override;
 
   bool setFPMath(StringRef Name) override;
 
+  bool supportsExtendIntArgs() const override {
+    return getTriple().getArch() != llvm::Triple::x86;
+  }
+
   CallingConvCheckResult checkCallingConvention(CallingConv CC) const override {
     // Most of the non-ARM calling conventions are i386 conventions.
     switch (CC) {
     case CC_X86ThisCall:
     case CC_X86FastCall:
     case CC_X86StdCall:
     case CC_X86VectorCall:
     case CC_X86RegCall:

clang/lib/Driver/ToolChains/Clang.cpp

#endif
 
   // Handle segmented stacks.
   if (Args.hasFlag(options::OPT_fsplit_stack, options::OPT_fno_split_stack,
                    false))
     CmdArgs.push_back("-fsplit-stack");
 
   RenderFloatingPointOptions(TC, D, OFastEnabled, Args, CmdArgs, JA);
 
+  if (Arg *A = Args.getLastArg(options::OPT_fextend_args_EQ)) {
+    const llvm::Triple::ArchType Arch = TC.getArch();
+    if (Arch == llvm::Triple::x86 || Arch == llvm::Triple::x86_64) {
+      StringRef V = A->getValue();
+      if (V == "64")
+        CmdArgs.push_back("-fextend-arguments=64");
+      else if (V != "32")
+        D.Diag(diag::err_drv_invalid_argument_to_option)
+            << A->getValue() << A->getOption().getName();
+    } else
+      D.Diag(diag::err_drv_unsupported_opt_for_target)
+          << A->getOption().getName() << TripleStr;
+  }
+
   if (Arg *A = Args.getLastArg(options::OPT_mdouble_EQ)) {
     if (TC.getArch() == llvm::Triple::avr)
       A->render(Args, CmdArgs);
     else
       D.Diag(diag::err_drv_unsupported_opt_for_target)
           << A->getAsString(Args) << TripleStr;
   }
 

clang/lib/Sema/SemaExpr.cpp

if (getLangOpts().OpenCL &&
         !getOpenCLOptions().isAvailableOption("cl_khr_fp64", getLangOpts())) {
       if (BTy->getKind() == BuiltinType::Half) {
         E = ImpCastExprToType(E, Context.FloatTy, CK_FloatingCast).get();
       }
     } else {
       E = ImpCastExprToType(E, Context.DoubleTy, CK_FloatingCast).get();
     }
   }
+  if (BTy &&
+      getLangOpts().getExtendIntArgs() ==
+          LangOptions::ExtendArgsKind::ExtendTo64 &&
+      Context.getTargetInfo().supportsExtendIntArgs() && Ty->isIntegerType() &&
+      Context.getTypeSizeInChars(BTy) <
+          Context.getTypeSizeInChars(Context.LongLongTy)) {
+    E = (Ty->isUnsignedIntegerType())
+            ? ImpCastExprToType(E, Context.UnsignedLongLongTy, CK_IntegralCast)
+                  .get()
+            : ImpCastExprToType(E, Context.LongLongTy, CK_IntegralCast).get();
+    assert(8 == Context.getTypeSizeInChars(Context.LongLongTy).getQuantity() &&
+           "Unexpected typesize for LongLongTy");
+  }
 
   // C++ performs lvalue-to-rvalue conversion as a default argument
   // promotion, even on class types, but note:
   //   C++11 [conv.lval]p2:
   //     When an lvalue-to-rvalue conversion occurs in an unevaluated
   //     operand or a subexpression thereof the value contained in the
   //     referenced object is not accessed. Otherwise, if the glvalue
   //     has a class type, the conversion copy-initializes a temporary

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

+// RUN: %clang_cc1 -DD128 -triple x86_64-apple-darwin -fextend-arguments=64  \
+// RUN:            %s -emit-llvm -o - | FileCheck %s -check-prefix=CHECKEXT
+
+// When the option isn't selected, no effect
+// RUN: %clang_cc1 -DD128 -triple x86_64-apple-darwin  \
+// RUN:                     %s -emit-llvm -o - | FileCheck %s \
+// RUN:    --implicit-check-not "ext {{.*}}to i64"
+
+// The option isn't supported on x86, no effect
+// RUN: %clang_cc1 -triple i386-pc-linux-gnu -fextend-arguments=64 \
+// RUN:                     %s -emit-llvm -o - | FileCheck %s \
+// RUN:    --implicit-check-not "ext {{.*}}to i64"
+
+// The option isn't supported on ppc, no effect
+// RUN: %clang_cc1 -triple ppc64le -fextend-arguments=64 \
+// RUN:                     %s -emit-llvm -o - | FileCheck %s \
+// RUN:    --implicit-check-not "ext {{.*}}to i64"
+
+int vararg(int, ...);
+void knr();
+
+unsigned int u32;
+int s32;
+unsigned short u16;
+short s16;
+unsigned char u8;
+signed char s8;
+long long ll;
+_ExtInt(23) ei23;
+float ff;
+double dd;
+#ifdef D128
+__int128 i128;
+#endif
+
+int test() {
+  // CHECK: define{{.*}} i32 @test{{.*}}
+
+  // CHECKEXT:  [[TAG_u32:%.*]] = load i32, i32* @u32{{.*}}
+  // CHECKEXT: [[CONV_u32:%.*]] = zext i32 [[TAG_u32]] to i64
+
+  // CHECKEXT:  [[TAG_s32:%.*]] = load i32, i32* @s32
+  // CHECKEXT: [[CONV_s32:%.*]] = sext i32 [[TAG_s32]] to i64
+
+  // CHECKEXT:  [[TAG_u16:%.*]] = load i16, i16* @u16
+  // CHECKEXT: [[CONV_u16:%.*]] = zext i16 [[TAG_u16]] to i64
+
+  // CHECKEXT:  [[TAG_s16:%.*]] = load i16, i16* @s16
+  // CHECKEXT: [[CONV_s16:%.*]] = sext i16 [[TAG_s16]] to i64
+
+  // CHECKEXT:  [[TAG_u8:%.*]] = load i8, i8* @u8
+  // CHECKEXT: [[CONV_u8:%.*]] = zext i8 [[TAG_u8]] to i64
+
+  // CHECKEXT:  [[TAG_s8:%.*]] = load i8, i8* @s8
+  // CHECKEXT: [[CONV_s8:%.*]] = sext i8 [[TAG_s8]] to i64
+  // CHECKEXT: call{{.*}} @vararg(i32 %0, i64 [[CONV_u32]], i64 [[CONV_s32]], i64 [[CONV_u16]], i64 [[CONV_s16]], i64 [[CONV_u8]], i64 [[CONV_s8]]
+
+  int sum = 0;
+  sum = vararg(sum, u32, s32, u16, s16, u8, s8);
+  knr(ll);
+  // CHECKEXT: load i64, i64* @ll
+  // CHECKEXT-NEXT: call void (i64, ...) bitcast {{.*}} @knr
+
+  knr(ei23);
+  // CHECKEXT: load i23, i23* @ei23
+  // CHECKEXT-NEXT: call void (i23, ...) bitcast{{.*}} @knr
+
+  knr(ff);
+  // CHECKEXT: load float
+  // CHECKEXT-NEXT: fpext float {{.*}} to double
+  // CHECKEXT-NEXT: call{{.*}} void (double, ...) bitcast{{.*}} @knr
+
+  knr(dd);
+  // CHECKEXT: load double
+  // CHECKEXT-NEXT: call{{.*}} void (double, ...) bitcast{{.*}} @knr
+
+#ifdef D128
+  knr(i128);
+  // CHECKEXT: load i128
+  // CHECKEXT: call{{.*}} void (i64, i64, ...) bitcast{{.*}} @knr
+#endif
+
+  knr(u32, s32, u16, s16, u8, s8);
+  // CHECKEXT:  [[TAg_u32:%.*]] = load i32, i32* @u32{{.*}}
+  // CHECKEXT: [[CONv_u32:%.*]] = zext i32 [[TAg_u32]] to i64
+
+  // CHECKEXT:  [[TAg_s32:%.*]] = load i32, i32* @s32
+  // CHECKEXT: [[CONv_s32:%.*]] = sext i32 [[TAg_s32]] to i64
+
+  // CHECKEXT:  [[TAg_u16:%.*]] = load i16, i16* @u16
+  // CHECKEXT: [[CONv_u16:%.*]] = zext i16 [[TAg_u16]] to i64
+
+  // CHECKEXT:  [[TAg_s16:%.*]] = load i16, i16* @s16
+  // CHECKEXT: [[CONv_s16:%.*]] = sext i16 [[TAg_s16]] to i64
+
+  // CHECKEXT:  [[TAg_u8:%.*]] = load i8, i8* @u8
+  // CHECKEXT: [[CONv_u8:%.*]] = zext i8 [[TAg_u8]] to i64
+
+  // CHECKEXT:  [[TAg_s8:%.*]] = load i8, i8* @s8
+  // CHECKEXT: [[CONv_s8:%.*]] = sext i8 [[TAg_s8]] to i64
+  // CHECKEXT: call{{.*}} void (i64, i64, i64, i64, i64, i64, ...) bitcast{{.*}} @knr
+  return sum;
+}

clang/test/Driver/fextend-args.c

+// Options for intel arch
+// RUN: %clang -### -target x86_64-apple-darwin -fextend-arguments=32 %s 2>&1 \
+// RUN: | FileCheck --implicit-check-not "-fextend-arguments=32"  %s
+// RUN: %clang -### -target x86_64-apple-darwin -fextend-arguments=64 %s 2>&1 \
+// RUN: | FileCheck -check-prefix=CHECK-64 %s
+
+// Unsupported target
+// RUN: not %clang -target aarch64-unknown-windows-msvc -fextend-arguments=32 %s 2>&1 \
+// RUN: | FileCheck -check-prefix=UNSUPPORTED-TARGET %s
+
+// Invalid option value
+// RUN: not %clang -target x86_64-apple-darwin -fextend-arguments=0 %s 2>&1 \
+// RUN: | FileCheck -check-prefix=INVALID-VALUE %s
+
+// CHECK-64: "-cc1" {{.*}}"-fextend-arguments=64"
+// UNSUPPORTED-TARGET: error: unsupported option
+// INVALID-VALUE: error: invalid argument '0' to -fextend-arguments