[PATCH] D94458: [PowerPC] Only use some extend mne if assembler is modern enough

Thu Jan 14 08:47:35 PST 2021

sfertile added inline comments.

================
Comment at: llvm/lib/Target/PowerPC/PPC.td:60
                                         "Enable 64-bit instructions">;
+def FeatureModernAs     : SubtargetFeature<"modern-as","HasModernAs", "true",
+                                        "Has new Assembler" >;
----------------
jsji wrote:
> sfertile wrote:
> > jsji wrote:
> > > sfertile wrote:
> > > > Minor nit: I think name, option, description string, etc should explicitly reflect its the AIX assembler.
> > > I updated the description and comments to mention AIX, but not the naming of feature. 
> > > As I think we should leave this as a general feature that can be used in the future for similar workaround, not necessary AIX only.
> > If we find a bug in gas that necessitate disabling an alias and we attempt to use this same feature it would mean also disabling `xxspltd` and `mtudscr` on Linux breaking compatibility with gas. Likewise we would be disabling said alias on AIX breaking compatibility with the AIX system as for a gas bug. 
> Yes, unless we want to give one feature for each assembler limitation , there will be situation like, compiler will try to be conservative. 
If we were only talking about what the compiler emits then I would agree with you 100%. But the Feature also affects what the integrated assembler accepts, in which case breaking compatability due to bugs on another platform isn't acceptable. 

There might be another approach though: we don't need to disable accepting the bugged/missing mnemonics in the integrated assembler (ie we always add +modern-as to the MCSubtargetInfo).  Then we can have the conservative behaviour in what the compiler emits, still accept anything the reference assembler does, and not have to have multiple Features to control the behaviour. The down side is there is assembly integrated-as will accept that the reference assembler won't.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94458