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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a comment.

A few more comments need to be addressed for this to proceed.



================
Comment at: llvm/lib/Target/PowerPC/AsmParser/PPCAsmParser.cpp:360
+  bool isS34Imm() const {
+    return Kind == Expression || (Kind == Immediate && isInt<34>(getImm()));
+  }
----------------
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.
```


================
Comment at: llvm/lib/Target/PowerPC/Disassembler/PPCDisassembler.cpp:336
                                              raw_ostream &CS) const {
+  auto *ReadFunc = IsLittleEndian ? support::endian::read32le
+                                  : support::endian::read32be;
----------------
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.


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


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


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrPrefix.td:82
+  let Inst{8-10} = 0;
+  let Inst{11} = 0;
+  let Inst{12-13} = 0;
----------------
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.


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


================
Comment at: llvm/lib/Target/PowerPC/PPCSubtarget.h:109
+  bool HasPrefixInstrs;
+  bool HasPCRelativeMemops;
   bool HasFCPSGN;
----------------
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.


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


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

https://reviews.llvm.org/D72569