[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:
> MattDevereau wrote:
> > 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.
> Sorry I didn't express it clearly.
> For example,
> The add operation is src + (100000)b
> I mean that what if the src has an enabled bit at the index corresponding to the '1' in the addedVal, index 5 
> so that means there will be 1+1 and then index 6 will be affected.
> How that case is handled ?
Sorry but I don't understand what you mean by case. I think what you are referring to is the rounding behaviour of rshrnb which is the intended calculation and not a side effect.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155299