[PATCH] D95115: [PowerPC] Update Refactored Load/Store Implementation, XForm VSX Patterns, and Tests

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai requested changes to this revision.
nemanjai added a comment.
This revision now requires changes to proceed.

It is really nice to see us getting rid of all these unnecessary X-Forms in so many test cases. But I think the patch needs a bit more tweaking.

Also, it seems that if we commit the first patch in this series before we commit this one, there is a possibility of emitting an impossible DSForm/DQForm instruction (since the alignment check for the FI is only added here). Can you explain why that is not the case? I think for DQ-Form it is because we only produce DQ-Forms starting with this patch, but I think it is possibly a problem with DS-Form.



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:16812
 
+  auto IsFrameIndexAligned = [&](SDValue N) {
+    // Set alignment flags based on whether or not the Frame Index is aligned.
----------------
Similarly to my comment on the other lambda in the first patch in the series, please turn this lambda into a static function. Also, the name is not appropriate. If a function is named as a query, it should be a query - calling `IsFrameIndexAligned()` seems like the caller is asking a question and would expect an answer.
Perhaps `setAlignFlagsForFI()`?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:16814
+    // Set alignment flags based on whether or not the Frame Index is aligned.
+    bool IsAdd = N.getOpcode() == ISD::ADD;
+    if (FrameIndexSDNode *FI =
----------------
How about if the address is computed using `OR`? I am fairly certain that the original code handles this case.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:16819
+      unsigned FrameIndexVal = MFI.getObjectAlign(FI->getIndex()).value();
+      if ((FrameIndexVal % 4) != 0)
+        FlagSet &= ~PPC::MOF_RPlusSImm16Mult4;
----------------
A comment explaining something like this seems appropriate:
```
If this is (add $FI, $S16Imm), the alignment flags are already set based on the immediate. We just need to clear the alignment flags if the FI alignment is weaker.
```


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:16824
+
+      if (!IsAdd) {
+        if ((FrameIndexVal % 4) == 0)
----------------
```
// If the address is a plain FrameIndex, set alignment flags based on FI alignment.
```


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:16939
+  // an aligned DForm when the frame index is not aligned.
+  if (FrameIndexSDNode *FI = dyn_cast<FrameIndexSDNode>(N)) {
+    PPC::AddrMode Mode = getAddrModeForFlags(FlagSet);
----------------
I don't really follow why we need this at all. This function is meant to set the flags. This preemptive look at which addressing mode we will produce for the flags we computed so far seems out of place.
Presumably simply calling `IsFrameIndexAligned(N)` ensure that `PPC::MOF_RPlusSImm16Mult4/FlagSet & PPC::MOF_RPlusSImm16Mult16` are not set on unaligned frame indices. And presumably, if those flags are not set, we'd never try to match `AM_DSForm/AM_DQForm` and would simply fall back to `AM_XForm`. It might help if you provide a test case that requires this code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95115