[PATCH] D143283: [AArch64][combine]: combine lshr pattern. [WIP]

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17600
 
+  // If we see something like trunc( lshr( add(add(a, b), c) , 1) ), where one of (a,b,c) should be 0 or 1 then
+  // we can convert that into lshr(a, 1) + lshr(b, 1) + (a|b)&1
----------------
As I understand it, the point to simplify the following:

     trunc(lshr(add(add(ext(a), ext(b)), 1), 1))
  -> lshr(a, 1) + lshr(b, 1) + (a | b) & 1

  iff the type of ext(a) is not a legal type


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17602
+  // we can convert that into lshr(a, 1) + lshr(b, 1) + (a|b)&1
+  if(!Subtarget->hasSVE2() && N->getOperand(0).getOpcode() == ISD::SRL) {
+    SDValue Op0 = N->getOperand(0).getOperand(0); //add(add(a, b), c)
----------------
I see that in llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp it tries to combine this pattern into a simpler `AVGFLOORS/AVGFLOORU/AVGCEILS/AVGCEILU` node in the function `combineShiftToAVG`. Is there a way to re-use this existing mechanism and then Custom lower this node to the desired set of instructions? I even wonder if we could generalise this code (i.e. not specific to AArch64), when the nodes are set to Expand.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17602
+  // we can convert that into lshr(a, 1) + lshr(b, 1) + (a|b)&1
+  if(!Subtarget->hasSVE2() && N->getOperand(0).getOpcode() == ISD::SRL) {
+    SDValue Op0 = N->getOperand(0).getOperand(0); //add(add(a, b), c)
----------------
sdesmalen wrote:
> I see that in llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp it tries to combine this pattern into a simpler `AVGFLOORS/AVGFLOORU/AVGCEILS/AVGCEILU` node in the function `combineShiftToAVG`. Is there a way to re-use this existing mechanism and then Custom lower this node to the desired set of instructions? I even wonder if we could generalise this code (i.e. not specific to AArch64), when the nodes are set to Expand.
You're testing for SVE2 (vector extension), but the test is using scalar types, not vector types. That doesn't seem entirely right?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:18572
 
+  // If we see something like zext( lshr( add(add(a, b), 1) , 1) ) then
+  // we can convert that into lshr(a, 1) + lshr(b, 1) + (a|b)&1
----------------
I don't believe this is a transform we want to make.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143283