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

[Stefan Pintilie via Phabricator via llvm-commits]

stefanp added a comment.

I just had some minor comments. I think it makes sense overall.



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:2491
+
+  Imm = (int32_t)cast<ConstantSDNode>(N)->getZExtValue();
+  if (N->getValueType(0) == MVT::i32)
----------------
I find it odd that you are zero extending this (ie unsigned) and then casting it to a signed.
You can get into all kinds of issues with this kind of thing.
For example:
```
int16_t a = -1;  // This is 0xFFFF
int32_t b = zeroExtend(a); // This is 0x0000FFFF (Not -1 but 65535)
```
It is important to think about what the possible value types are for `N`.  If the only possible types are `MVT::i32` and `MVT::i64` then we are fine.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:2495
+  else
+    return Imm == (int64_t)cast<ConstantSDNode>(N)->getZExtValue();
+}
----------------
You can probably simplify this a little bit. See what others think too...
```
Imm = (int32_t)cast<ConstantSDNode>(N)->getZExtValue();
int64_t Imm64 = (int64_t)cast<ConstantSDNode>(N)->getZExtValue();
return isInt<32>(Imm64);
```


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:17252
+    else // Let constant materialization handle large constants.
+      FlagSet |= PPC::MOF_NotAdd;
+  } else if (N.getOpcode() == ISD::ADD || provablyDisjointOr(DAG, N)) {
----------------
For this case it may be easier to just work with the APInt instead of creating your own functions.
So, you may not need `isIntS32Immediate` and `isIntS34Immediate`. Especially since you don't use Imm34.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93370