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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
