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