[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:2988-2992
+.. option:: -mconservative-extend
+Always extend the integer parameter both in the callee and caller.
+
+.. option:: -mno-conservative-extend
+Keep the original integer parameter passing behavior.
----------------
pengfei wrote:
> Combine like others?
How about:

```
In the past, Clang passed small integer arguments on certain targets using a
parameter convention in which the caller was assumed to have sign-extended
or zero-extended the argument to a certain width.  This convention was not
conformant with the documented ABI on these platforms, which does not
require the caller to perform this extension.  Clang no longer assumes that
callers perform this extension, but for compatibility with code compiled by
previous releases of Clang, Clang defaults to still extending the argument in the
caller.  `-mno-conservative-extend` disables this, which may improve
performance and code size if compatibility with old versions of Clang is not
required.

This affects most 32-bit and 64-bit x86 targets, except:
- Windows, which Clang has never assumed extension on
- Apple platforms, which use a non-standard ABI that unconditionally assumes extension
```

Note that I need to check that that's what Apple actually wants to do.  You should also reach out to the Sony folks to see what they want to do, but I expect that it's to assume extension unconditionally.


================
Comment at: clang/include/clang/Basic/CodeGenOptions.def:475
 
+/// Whether the ingeter parameter should be extended in callee and caller.
+CODEGENOPT(ConservativeExtend, 1, 1)
----------------
"Whether callers should extend small integer parameters even though callees are not supposed to expect extension."


================
Comment at: clang/include/clang/CodeGen/CGFunctionInfo.h:48
+    /// ConservativeExtend - Valid only for integer argument types. Same as
+    /// 'Extend' but do not make any assumptions about the caller and callee.
+    ConservativeExtend,
----------------
"...but do not assume in the callee that the caller actually did the extension."


================
Comment at: clang/include/clang/CodeGen/CGFunctionInfo.h:120
   bool SRetAfterThis : 1;   // isIndirect()
   bool InReg : 1;           // isDirect() || isExtend() || isIndirect()
   bool CanBeFlattened: 1;   // isDirect()
----------------
rnk wrote:
> Instead of introducing a new ABIInfo::Kind value, should this be a boolean flag that is part of ABIInfo::Extend? That seems like it would be simpler.
I think adding a new `Kind` is better; it's a significantly different ABI that clients should be forced to handle correctly, which they will not if it's just a flag.


================
Comment at: clang/include/clang/Driver/Options.td:3335
+  HelpText<"Always extend the integer parameter both in the callee and caller."
+           "Do not make any assumptions about the callee and caller">;
+def mno_conservative_extend : Flag<["-"], "mno-conservative-extend">, Group<m_Group>,
----------------
The callee isn't really extending anything; please the wording I suggest above.


================
Comment at: clang/lib/CodeGen/CGCall.cpp:1617
   case ABIArgInfo::IndirectAliased:
+  case ABIArgInfo::ConservativeExtend:
     llvm_unreachable("Invalid ABI kind for return argument");
----------------
Please write the code to work correctly for return types even if we aren't using it anywhere yet.


================
Comment at: clang/lib/CodeGen/CGCall.cpp:2450
+      // ConservativeExtend. In this case, we should only add sext/zext
+      // attribute to the callee.
+      if (AttrOnCallSite || AI.getKind() == ABIArgInfo::Extend) {
----------------
"Under ConservativeExtend, add sext/zext only on the call site; the callee does not get to assume extension."


================
Comment at: clang/lib/CodeGen/CGCall.cpp:3655
   case ABIArgInfo::IndirectAliased:
+  case ABIArgInfo::ConservativeExtend:
     llvm_unreachable("Invalid ABI kind for return argument");
----------------
Please make this correct when used on return types.


================
Comment at: clang/lib/CodeGen/CGCall.cpp:5573
     case ABIArgInfo::IndirectAliased:
+    case ABIArgInfo::ConservativeExtend:
       llvm_unreachable("Invalid ABI kind for return argument");
----------------
Please make this correct when used on return types.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:1938
+    return IsConservativeExtend ? ABIArgInfo::getConservativeExtend(Ty)
+                                : ABIArgInfo::getExtend(Ty);
   }
----------------
This looks wrong.  In non-`ConservativeExtend` mode, we don't get to assume extension at all and should use `Direct`, right?


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:3818
+          return ABIArgInfo::getConservativeExtend(Ty);
         return ABIArgInfo::getExtend(Ty);
+      }
----------------
Same comment: this should use `Direct` when we're not in `ConservativeExtend` mode, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124435