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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai accepted this revision.
nemanjai added a comment.
This revision is now accepted and ready to land.

The rest of the comments I have are minor comment and code restructuring nits. Those can be addressed on the commit. Otherwise, LGTM.



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:16896
+  bool IsAdd = ((N.getOpcode() == ISD::ADD) || (N.getOpcode() == ISD::OR));
+  if (FrameIndexSDNode *FI =
+      dyn_cast<FrameIndexSDNode>(IsAdd ? N.getOperand(0) : N)) {
----------------
Please reduce nesting by flipping this and early-exiting:
```
FrameIndexSDNode *FI =
  dyn_cast<FrameIndexSDNode>(IsAdd ? N.getOperand(0) : N);
if (!FI)
  return;
// The rest of the code is not nested in this if
```


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:16899
+    const MachineFrameInfo &MFI = DAG.getMachineFunction().getFrameInfo();
+    unsigned FrameIndexVal = MFI.getObjectAlign(FI->getIndex()).value();
+    // If this is (add $FI, $S16Imm), the alignment flags are already set
----------------
It might be clear to call this something like `FrameIndexAlign` since it seems to refer to alignment rather than a value.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:17127
+  case PPC::AM_DQForm: {
+    // Can represent as an ADD.
+    if (Flags & PPC::MOF_RPlusSImm16) {
----------------
```
// This is a register plus a 16-bit immediate. The base will be the
// register and the displacement will be the immediate unless it
// isn't sufficiently aligned.
```


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:17134-17139
+        Disp = DAG.getTargetConstant(Imm, DL, N.getValueType());
+        if (FrameIndexSDNode *FI = dyn_cast<FrameIndexSDNode>(Op0)) {
+          Base = DAG.getTargetFrameIndex(FI->getIndex(), N.getValueType());
+          fixupFuncForFI(DAG, FI->getIndex(), N.getValueType());
+        } else
+          Base = Op0;
----------------
```
Disp = DAG.getTargetConstant(Imm, DL, N.getValueType());
Base = Op0;
if (FrameIndexSDNode *FI = dyn_cast<FrameIndexSDNode>(Op0)) {
  Base = DAG.getTargetFrameIndex(FI->getIndex(), N.getValueType());
  fixupFuncForFI(DAG, FI->getIndex(), N.getValueType());
}
```


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:17143
+    }
+    // Match LOAD (ADD (X, Lo(G))).
+    else if (Flags & PPC::MOF_RPlusLo) {
----------------
This is not necessarily a load I think.
```
// This is a register plus the @lo relocation. The base is the register
// and the displacement is the global address.
```


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:17153
+    }
+    // Match 16-bit and 32-bit constant addresses.
+    else if (Flags & PPC::MOF_AddrIsSImm32) {
----------------
```
// This is a constant address at most 32 bits. The base will be
// zero or load-immediate-shifted and the displacement will be
// the low 16 bits of the address.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93370