rjmccall added inline comments.
================ Comment at: clang/lib/CodeGen/TargetInfo.cpp:1938 + return IsConservativeExtend ? ABIArgInfo::getConservativeExtend(Ty) + : ABIArgInfo::getExtend(Ty); } ---------------- 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. 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
