[PATCH] D54409: PowerPC/SPE: Fix load/store handling for SPE

[Nemanja Ivanovic via Phabricator via llvm-commits]

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

I am only requesting changes for the refactoring of the function. As I mentioned in the inline comment, I am not necessarily requesting that you rework the addressing mode computation functions as I mentioned, just that you clean up the code in `SelectAddressRegReg()`.



================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:2213
   if (N.getOpcode() == ISD::ADD) {
+    if (hasSPE()) {
+      // Is there any SPE load/store (f64) which can't handle 16bit offset?
----------------
I really don't think this belongs here. This should probably be in another function. Perhaps something like `SelectAddressEVXRegReg(...)`. This function would need to decide when r+r mode needs to be used.

Furthermore, it is probably buggy that we use `iaddr` for `EVLDD` because it might go into a function like this, the node that computes the address is not an `ISD::ADD` and it reverts to default handling (which is set up to handle 16-bit displacements). Even if you added the code to the `ISD::OR` section below, it is possible that we would get it wrong as the address came from a different node that `SelectAddressRegImm()` ends up knowing about.

Ultimately, this is a different addressing mode and it should use different functions to compute when to use the r+r form and when to use r+i form.

That being said, I understand that this solves a current problem whereas what I am suggesting solves a future problem that may never actually occur. So I won't stand in the way of this patch - you just might want to think about re-working this.

At the very least, this code needs to be pulled out into a separate function and this can look like:
```
if (hasSPE() && usedForDPMemOp(N)) {
  Base = N.getOperand(0);
  Index = N.getOperand(1);
  return true;
```


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:2217
+          UI != E; ++UI) {
+        if (UI->getOpcode() == ISD::STORE) {
+          // Store has the type Operand[1]
----------------
This is not necessary. You should be able to do something like:
```
if (MemSDNode *Memop = dyn_cast<MemSDNode>(*UI)) {
  if (Memop->getMemoryVT() != MVT::f64)
    // ...
}


================
Comment at: lib/Target/PowerPC/PPCRegisterInfo.cpp:969
          "This should be handled in a target-independent way");
-  if (!noImmForm && ((isInt<16>(Offset) &&
+  bool canBeImmediate = (OpC == PPC::EVSTDD || OpC == PPC::EVLDD) ?
+                        isUInt<8>(Offset) :
----------------
A more descriptive name might be something like `OffsetFitsMnemonic`. Notice the capitalization of the variable name as per coding guidelines.


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

https://reviews.llvm.org/D54409