[PATCH] D93370: [PowerPC] Add new infrastructure to select load/store instructions, update P8/P9 load/store patterns.

[Baptiste Saleil via Phabricator via llvm-commits]

bsaleil added a comment.

Thanks a lot for working on that Amy ! I have some comments on the patch.



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:17192
+/// the address flags of the load/store instruction that is to be matched.
+/// The address flags are are stored in a map, which is then searched through
+/// to determine the optimal load/store instruction format to match by.
----------------
`The address flags are are stored` -> `The address flags are stored`


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:17199-17200
+  // Compute subtarget flags.
+  const PPCSubtarget &Subtarget =
+      static_cast<const PPCSubtarget &>(DAG.getSubtarget());
+  if (!Subtarget.hasP9Vector())
----------------
I think you can directly use the `Subtarget` field of the `PPCTargetLowering` class instead of using a `static_cast` here.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:17213
+  // or pre-increment instruction.
+  const MemSDNode *MN = dyn_cast<MemSDNode>(Parent);
+  if (const LSBaseSDNode *LSB = dyn_cast<LSBaseSDNode>(Parent))
----------------
This line should be moved below just before the assert.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:17260
+    const APInt &ConstImm = CN->getAPIntValue();
+    if (ConstImm.isSignedIntN(32)) { // Flag to handle 32-bit consstants.
+      FlagSet |= PPC::MOF_AddrIsSImm32;
----------------
`consstants` -> `constants`


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:17289-17290
+      FlagSet |= PPC::MOF_RPlusR;
+  } else // The address computation is not a constant or an addition.
+    FlagSet |= PPC::MOF_NotAdd;
+
----------------
Should we name this flag `MOF_NotAddNorCst` then ?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:17338-17340
+  if (N.getOpcode() == ISD::ADD &&
+      (!isIntS16Immediate(N.getOperand(1), ForceXFormImm) ||
+       !N.getOperand(1).hasOneUse() || !N.getOperand(0).hasOneUse())) {
----------------
I know this is code we already have in `SelectAddressRegRegOnly`, but isn't that condition weird ? Or at least it doesn't match the comment above. If I understand correctly, this is the case where we get rid of the `add`, but looking at the condition, we get rid of it only if it is not an add of a value and a 16-bit signed constant or if one of the operands doesn't have a single use.
Shouldn't the condition be `(N.getOpcode() == ISD::ADD && !isIntS16Immediate(N.getOperand(1), ForceXFormImm) && N.getOperand(1).hasOneUse() && N.getOperand(0).hasOneUse())` instead ?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.h:691-696
+      MOF_SubWInt = 1 << 15,
+      MOF_WordInt = 1 << 16,
+      MOF_DWInt = 1 << 17,
+      MOF_ScalFlt = 1 << 18,
+      MOF_Vec = 1 << 19,
+      MOF_Vec256 = 1 << 20,
----------------
The flag names are not really uniform, I guess DWInt is for DoubleWordInt ? I think we should drop the abbreviation here so it is more clear what the flags represent. Something like:

```
MOF_SubWordInt = 1 << 15,
MOF_WordInt = 1 << 16,
MOF_DoubleWordInt = 1 << 17,
MOF_ScalarFloat = 1 << 18,
MOF_Vector = 1 << 19,
MOF_Vector256 = 1 << 20,
```


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.h:699
+      // Subtarget features.
+      MOF_SubtargetNoP9 = 1 << 22,
+      MOF_SubtargetP9 = 1 << 23,
----------------
Maybe we should name this flag `MOF_SubtargetBeforeP9` or something like that, to make it clear that the flag is not set on P10.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.h:718-719
+    // Map that relates a set of common address flags to PPC addressing modes.
+    std::map<PPC::AddrMode, SmallVector<unsigned, 16>> AddrModesMap;
+    void initializeAddrModeMap();
 
----------------
I think the map and the function declaration should be moved with the others private fields below.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.h:1087
+    /// optimal instruction format to match by.
+    PPC::AddrMode getAddrModeForFlags(unsigned Flags) const;
+    /// computeMOFlags - Given a node N and it's Parent (a MemSDNode), compute
----------------
This method should be private.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.h:1092
+    /// through to determine the optimal load/store instruction format.
+    unsigned computeMOFlags(const SDNode *Parent, SDValue N,
+                            SelectionDAG &DAG) const;
----------------
This method should be private.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93370