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

[Amy Kwan via Phabricator via llvm-commits]

amyk marked 10 inline comments as done.
amyk added inline comments.


================
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())) {
----------------
bsaleil wrote:
> 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 ?
That's a good point, Baptiste. You're right in that this is the case where we eliminate the `add`. 

I thought about it a bit I believe the this condition might match the comment more (only get rid of the `add` if we **do not** have an `add` of a value and a signed 16-bit immediate, and the two operands don't have a single use)
```
if (N.getOpcode() == ISD::ADD &&
      (!(isIntS16Immediate(N.getOperand(1), imm) &&
         N.getOperand(1).hasOneUse() && N.getOperand(0).hasOneUse())))
```
which I believe, should then be equivalent to the condition in the code. 

But yes, the condition was previously in `SelectAddressRegRegOnly()` which is why I kept it. 
If there are any more concerns on the condition and/or comment, I can probably adjust it. 


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

https://reviews.llvm.org/D93370