[PATCH] D112413: [X86] Add -mskip-rax-setup support to align with GCC

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers accepted this revision.
nickdesaulniers added a comment.
This revision is now accepted and ready to land.

Thanks for the feature.  I'd still prefer to see `SimpleMFlag` used, but it's not a big deal to me.



================
Comment at: clang/include/clang/Driver/Options.td:3193
+def mskip_rax_setup : Flag<["-"], "mskip-rax-setup">, Group<m_Group>, Flags<[NoXarchOption]>,
+  HelpText<"Skip setting up RAX register when passing variable arguments (x86 only)">;
 def mstackrealign : Flag<["-"], "mstackrealign">, Group<m_Group>, Flags<[CC1Option]>,
----------------
pengfei wrote:
> nickdesaulniers wrote:
> > pengfei wrote:
> > > MaskRay wrote:
> > > > nickdesaulniers wrote:
> > > > > I think GCC support `-mno-skip-rax-setup` as well. Can you please add that (and tests for it) as well?  We don't need to actually handle it, I think, but we shouldn't warn about the flag being unsupported, for example.
> > > > Consider `SimpleMFlag`
> > > Thanks for the suggestion. I think adding a `-mno-skip-rax-setup` is simply here.
> > Via @MaskRay 's suggestion, can we do something like:
> > 
> > ```
> > def skip_rax_setup : SimpleMFlag<"skip-rax-setup", "Skip", "Do not skip", "setting up RAX register when passing variable arguments (x86 only)">;
> > ```
> > 
> > Do we test that this flag is warned about being unused for non-x86 targets? (Is it worth adding such a test to clang)?
> Another reason I didn't use `SimpleMFlag` is GCC doesn't have help test for `-mno-skip-rax-setup`. I deliberately hide it in this way.
> 
> > Do we test that this flag is warned about being unused for non-x86 targets? (Is it worth adding such a test to clang)?
> 
> I tested it locally. Driver will warn it for other target, Clang doesn't. I think it should be enough. I don't know if there's precedent, but adding an irrelevant option test to other target's test folder looks a bit weird to me.
> Another reason I didn't use SimpleMFlag is GCC doesn't have help test for -mno-skip-rax-setup. I deliberately hide it in this way.

While clang does try hard to emulate GCC, both in features and interface, that's trying too hard to emulate poor interface.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112413