[PATCH] D124435: [X86] Always extend the integer parameters in callee

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/docs/ClangCommandLineReference.rst:3028
+extend the small integer argument in both caller and callee. Using other
+value can disable this, which may improve performance and code size.
+
----------------
> Specifies how to handle small integer arguments on i386 and x86_64
> targets that generally follow the System V ABI.  The value must be ``none``,
> ``conservative``, ``assumed``, or ``default``. The default behavior
> depends on the target and can be explicitly requested with ``default``.
> 
> In the past, on System V i386 and x86_64 targets, Clang passed promotable
> integer arguments (non-variadic arguments with types smaller than 'int')
> using a parameter convention in which the caller was assumed to have
> sign-extended or zero-extended the argument to the width of an 'int'. This
> convention did not conform to the documented ABI for these targets, which
> does not require the caller to perform this extension.  This can introduce
> incompatibilities with code generated by other compilers on these targets.
>
> On most targets using the System V ABI, Clang no longer assumes that
> callers perform this extension.  In order to maintain compatibility with
> code compiled by old versions of Clang, Clang will still perform the extension
> in the caller.  This is the ``conservative`` behavior.  If compatibility with old
> Clang code is not required, extension in the caller can be disabled using
> ``none``, which may have minor performance and code-size benefits.
>
> On certain targets which use Clang as the system compiler, Clang continues
> to perform extension in the caller and assume it in in the callee.  This is the
> behavior of ``assumed``.  These platforms therefore diverge from the
> standard System V ABI.  If compatibility with other compilers which do not
> recognize this divergence is required, ``conservative`` may be used.
> Using ``none`` on these targets is allowed but may lead to ABI mismatches.
> These targets include Apple and PlayStation platforms.


================
Comment at: clang/include/clang/Driver/Options.td:3338
+           "'none' (Pass the small integer parameter directly in caller) | "
+           "'conserative' (Always extend small integer parameter in the caller but do not assume "
+           "in the callee that the caller actually did the extension) | "
----------------



================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:1938
+    return IsConservativeExtend ? ABIArgInfo::getConservativeExtend(Ty)
+                                : ABIArgInfo::getExtend(Ty);
   }
----------------
rjmccall wrote:
> LiuChen3 wrote:
> > rjmccall wrote:
> > > LiuChen3 wrote:
> > > > rjmccall wrote:
> > > > > This looks wrong.  In non-`ConservativeExtend` mode, we don't get to assume extension at all and should use `Direct`, right?
> > > > As I understand it, `Direct` means do nothing with the parameters. Caller won't do the extension and callee can't assume the parameter is correct. This makes new clang behave in the opposite way to currently clang behavior, which will cause incompatibility issue. e.g:
> > > > https://godbolt.org/z/d3Peq4nsG
> > > Oh, I see, you're thinking that `-mno-conservative-extend` means "do what old versions of clang did" rather than "break compatibility with old versions of clang and just follow the x86_64 ABI".  That's definitely different from the documentation I suggested, so something's got to change.
> > > 
> > > I think these are the possibilities here:
> > > 
> > > 1. Some platforms, like Apple's, are probably going to define Clang's current behavior as the platform ABI.  So those platforms need to continue to use `Extend`.  My previous comment about using `Direct` wasn't paying due attention to this case.
> > > 
> > > 2. On other platforms, we need to maintain compatibility by default with both the platform ABI and old Clang behavior.  Those platforms will need to use `ConservativeExtend`.
> > > 
> > > 3. Some people may want to opt out of (2) and just be compatible with the platform ABI, which has minor code-size and performance wins.  If we support that with an option, I believe it should cause us to emit `Direct`.  This is what I was thinking `-mno-conservative-extend` would mean.
> > > 
> > > 4. Some people may want to force the use of (1) even on platforms where that isn't the platform ABI.  I don't know if this is really something we should support in the long term, but it might be valuable for people staging this change in.  This is what you seem to be thinking `-mno-conservative-extend` would mean.
> > > 
> > > I would suggest these spellings for the argument, instead of making it boolean:
> > > 
> > > ```
> > > -mextend-small-integers=none    // Force the use of Direct
> > > -mextend-small-integers=conservative // Force the use of ConservativeExtend
> > > -mextend-small-integers=assumed // Force the use of Extend
> > > -mextend-small-integers=default // Use the default rule for the target
> > > ```
> > Yes. That's what I mean. I totally misunderstood your meaning before. 
> > I agree with your method. I will work on that. Just one concern about the 'default': the default behavior is 'conservative' instead of 'default'. Wouldn't that be a little weird? But with my limited English I can't think of a better word. 'default' is ok for me. :-)
> The default will be platform-specific.  For example, the de facto macOS x86_64 ABI is to extend in the caller, because clang is the system compiler, and that's what clang does.  Apple just needs to update its documentation.
Please go ahead and implement the target-specific logic.  Since Apple (and probably Sony) is opting out of it, it's incorrect to change the ABI there, even briefly.

I would suggest folding away the `Default` case so that you just deal with one of assumed/conservative/direct here, either when you construct the `ABIInfo` or maybe even when you parse the frontend options in `CompilerInvocation`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124435/new/

https://reviews.llvm.org/D124435