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

[Victor Huang via Phabricator via llvm-commits]

NeHuang marked 8 inline comments as done.
NeHuang added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/AsmParser/PPCAsmParser.cpp:360
+  bool isS34Imm() const {
+    return Kind == Expression || (Kind == Immediate && isInt<34>(getImm()));
+  }
----------------
nemanjai wrote:
> jsji wrote:
> > Do we have relocation similar to `@l`? Do we need to make this as `ContextImmediate`?
> I believe that all pc-relative relocations have only a single possible interpretation so we probably don't need a `ContextImmediate`. However, once the ABI is finalized we might want to revisit/reevaluate that. Perhaps just a FIXME in the code noting that?
> 
> ```
> // Once the PC-Rel ABI is finalized, evaluate whether a 34-bit
> // ContextImmediate is needed.
> ```
Will do. 


================
Comment at: llvm/lib/Target/PowerPC/Disassembler/PPCDisassembler.cpp:336
                                              raw_ostream &CS) const {
+  auto *ReadFunc = IsLittleEndian ? support::endian::read32le
+                                  : support::endian::read32be;
----------------
nemanjai wrote:
> kamaub wrote:
> > nit: Is it possible to use that actually type here instead of `auto`? I get that its hard to define because it's a template within a template but as far as i can tell in this use it would be `uint32_t (*ReadFunc) = ` this would make the code a more readable I think, selecting which function to use base on the target is more clear this way to me with this way.
> I think `auto` is preferable here as it does not restrict future changes to the interface of the functions themselves. For example, if at some point another parameter needed to be added with a default argument, hard-coding the function type here would cause compilation errors without a particularly good reason.
+1 


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


================
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;
----------------
nemanjai wrote:
> jsji wrote:
> > As mentioned in comments in line 33 above, we should add the enum in PPCInstrInfo.h as well.
> Yup!
Thanks @jsji, will do. 


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrPrefix.td:82
+  let Inst{8-10} = 0;
+  let Inst{11} = 0;
+  let Inst{12-13} = 0;
----------------
nemanjai wrote:
> kamaub wrote:
> > should this be `let Inst{11} = PCRel;` to match the definition in `MLS_DForm_R_SI34_RTA5` since `PI` sets it to zero and only `isPCRel` sets it to 1
> I think this one is explicitly not PC-Rel so that's why it's set to zero.
Agree, PC-Rel is not used in `PLI8` and `PLI`, should set zero here to avoid confusion.


================
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;
----------------
nemanjai wrote:
> jsji wrote:
> > It would be better to put these before `TSFlags{8}` so that they are in order and easier to check.
> +1
Will do 


================
Comment at: llvm/lib/Target/PowerPC/PPCSubtarget.h:109
+  bool HasPrefixInstrs;
+  bool HasPCRelativeMemops;
   bool HasFCPSGN;
----------------
nemanjai wrote:
> This of course also needs to go into the other patch. Also, both of these need to be initialized (to `false`) in the .cpp file.
Will do. 


================
Comment at: llvm/lib/Target/PowerPC/PPCSubtarget.h:267
+  bool hasPrefixInstrs() const { return HasPrefixInstrs; }
+  bool hasPCRelativeMemops() const { return HasPCRelativeMemops; }
   bool hasMFOCRF() const { return HasMFOCRF; }
----------------
nemanjai wrote:
> Same as above, please move to other patch.
Will do 


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

https://reviews.llvm.org/D72569