[PATCH] D155299: [AArch64][SVE2] Combine add+lsr to rshrnb for stores

[Matt Devereau via Phabricator via llvm-commits]

MattDevereau added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:20131
+
+  if (AddValue != 1 << (ShiftValue - 1))
+    return SDValue();
----------------
hassnaa-arm wrote:
> hassnaa-arm wrote:
> > Hi Mat,
> > I think that check doesn't consider the case when the AddValue has enabled bits other than the most significant bit.
> > ex: the ShiftValue is 6, and the AddValue is 33 (100001)b 
> > Is that correct ? 
> Do you handle the case when the destination of the AddOperation has an enabled bit in the same index of the enabled bit in the add Value ?
> In that case the Add operation will has side affect, and by combining that case into rshrnb, you cancel the side effect of the AddOperation.
> Do you agree on that ? or I'm missing something ??
>I think that check doesn't consider the case when the AddValue has enabled bits other than the most significant bit.
>ex: the ShiftValue is 6, and the AddValue is 33 (100001)b 

This check would correctly reject that example, which is not a case for this combine. As per [[ https://developer.arm.com/documentation/ddi0602/2023-06/SVE-Instructions/RSHRNB--Rounding-shift-right-narrow-by-immediate--bottom--?lang=en, | the documentation for rshrnb ]] the operation is 
> integer res = (UInt(element) + (1 << (shift-1))) >> shift;

As 33 is not possible to get from left shifting 1, the combine will correctly bail. The lower bits in this case would be kept as the combine wouldn't fire.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:20131
+
+  if (AddValue != 1 << (ShiftValue - 1))
+    return SDValue();
----------------
MattDevereau wrote:
> hassnaa-arm wrote:
> > hassnaa-arm wrote:
> > > Hi Mat,
> > > I think that check doesn't consider the case when the AddValue has enabled bits other than the most significant bit.
> > > ex: the ShiftValue is 6, and the AddValue is 33 (100001)b 
> > > Is that correct ? 
> > Do you handle the case when the destination of the AddOperation has an enabled bit in the same index of the enabled bit in the add Value ?
> > In that case the Add operation will has side affect, and by combining that case into rshrnb, you cancel the side effect of the AddOperation.
> > Do you agree on that ? or I'm missing something ??
> >I think that check doesn't consider the case when the AddValue has enabled bits other than the most significant bit.
> >ex: the ShiftValue is 6, and the AddValue is 33 (100001)b 
> 
> This check would correctly reject that example, which is not a case for this combine. As per [[ https://developer.arm.com/documentation/ddi0602/2023-06/SVE-Instructions/RSHRNB--Rounding-shift-right-narrow-by-immediate--bottom--?lang=en, | the documentation for rshrnb ]] the operation is 
> > integer res = (UInt(element) + (1 << (shift-1))) >> shift;
> 
> As 33 is not possible to get from left shifting 1, the combine will correctly bail. The lower bits in this case would be kept as the combine wouldn't fire.
What side effect do you mean exactly?

> Do you handle the case when the destination of the AddOperation has an enabled bit in the same index of the enabled bit in the add Value ?

I don't think the bits in the destination register of the add matter, register allocation should handle that fine and give us a non conflicting destination register.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:20136
+  return SDValue(
+      DAG.getMachineNode(Opc, DL, VT,
+                         {Add->getOperand(0),
----------------
dmgreen wrote:
> We should avoid creating machine nodes in SelectionDAG combines. It's like a layering violation. Can you change this to either generate an intrinsic, or preferably add a new AArch64ISD node for it?
Thanks for pointing that out. I'm working on emitting a new AArch64ISD node now, however it adds a bit more complexity to the patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155299