[PATCH] D72569: [PowerPC][Future] Add prefixed instruction paddi to future CPU

[Jinsong Ji via Phabricator via llvm-commits]

jsji added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/AsmParser/PPCAsmParser.cpp:360
+  bool isS34Imm() const {
+    return Kind == Expression || (Kind == Immediate && isInt<34>(getImm()));
+  }
----------------
Do we have relocation similar to `@l`? Do we need to make this as `ContextImmediate`?


================
Comment at: llvm/lib/Target/PowerPC/PPC.td:217
+                                            FeatureP9Altivec]>;
+def FeaturePCRelativeMemops :
+  SubtargetFeature<"pcrelative-memops", "HasPCRelativeMemops", "true",
----------------
Shouldn't this be in https://reviews.llvm.org/D72574 instead?


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrFormats.td:45
+  // Indicate that this instruction does not use the default register numbering.
+  bits<1> UseCustomRegNumbering = 0;
+  let TSFlags{8}  = UseCustomRegNumbering;
----------------
As mentioned in comments in line 33 above, we should add the enum in PPCInstrInfo.h as well.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrPrefix.td:34
+  /// these must be reflected there!  See comments there for what these are.
+  let TSFlags{0}   = PPC970_First;
+  let TSFlags{1}   = PPC970_Single;
----------------
It would be better to put these before `TSFlags{8}` so that they are in order and easier to check.


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

https://reviews.llvm.org/D72569