[PATCH] D106237: [ISel] Port AArch64 HADD and RHADD to ISel

Mon Jan 24 05:42:38 PST 2022

dmgreen added a comment.

In D106237#3263993 <https://reviews.llvm.org/D106237#3263993>, @craig.topper wrote:

>> Providing that the depth is limited, is there an issue with doing it as a demand bits combine?
>
> My main reason for questioning was because as far as I know this is a larger transform than anything else we do in SimplifiedDemandedBits. So it seemed a bit of a new direction for SimplifiedDemandedBits, but if we're not concerned about that then I don't object to this patch.

Yeah that's fair enough. I had considered it, and was hoping that the initial condition (`ConstantSDNode *N1C = isConstOrConstSplat(Op.getOperand(1)); if(N1C && N1C->isOne())..`) would rule most stuff out, limiting how expensive this is in practice. In my mind recursing into a SimplifiedDemandedBits call for "N" steps is going to be more expensive than any one transform inside it, due to the number of nodes potentially visited. But they will be related, and I have no hard evidence about which part is potentially slower.

================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:953
+  unsigned NumZeroA =
+      DAG.computeKnownBits(ExtOpA, Depth).countMinLeadingZeros();
+  unsigned NumZeroB =
----------------
craig.topper wrote:
> I'm not sure, but would it be better to use countMaxActiveBits and ComputeMaxSignificantBits. And do all the comparisons against the scalar bit width instead of against 1 and 2.
In my mind, saying an add requires a single extra bit is simpler than dealing with MaxActiveBits and BitWidths. But if you think it's worth changing I'm happy to change it.

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

https://reviews.llvm.org/D106237