[PATCH] D126532: [SVE] Add a DAG combiner fold to visitADD for vscale with truncate

[Allen zhong via Phabricator via llvm-commits]

Allen marked 2 inline comments as done.
Allen added a comment.

In D126532#3544144 <https://reviews.llvm.org/D126532#3544144>, @reames wrote:

> I don't claim to fully understand this, so my comment here might be off base.
>
> I suspect your fold can be generalized as: fold a+truncate(vscale(c1))+truncate(vscale(c2)) to a+truncate(vscale(c1)+vscale(c2))
>
> The vscale(c1)+vscale(c2) to vscale(C1 <https://reviews.llvm.org/C1> + C2) is handled separately above already.
>
> If this is true, that your transform reduces to proving that it's legal to common the truncate.  However, as a far as I known trunc(x) + trunc(y) is always equal to trunc(x+y).  So why do we need this transform at all?  Shouldn't this be covered by generic trunc folds and the existing rule?
>
> Anyways, I'm clearly missing something here.  Any idea what?

Thanks for your attention.  With my debug, I find vscale is a little especial. As the vscale(C1 <https://reviews.llvm.org/C1>) is not a const type node before LowerVSCALE, so we missing the transform of  **trunc(x) + trunc(y) --> trunc(x+y) ** with instcombine.
And in the dagcombine, it match the DAG IR from bottom to top, so we can capture the former **a+truncate(vscale(c1))+truncate(vscale(c2))**, but not **truncate(vscale(c1))+truncate(vscale(c2))**.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2622
       (N1.getOpcode() == ISD::VSCALE)) {
+    assert(VT == MVT::i64 && "Unexpected element type!");
     const APInt &VS0 = N0.getOperand(1)->getConstantOperandAPInt(0);
----------------
efriedma wrote:
> You can't assert that VSCALE returns an i64.  Probably not even on Arm, but definitely not in target-independent code.
Thanks, apply your commit


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2631
+  // to a+truncate(vscale(c1+c2))
+  if (VT.isScalarInteger() && VT.getSizeInBits() < 64 &&
+      (N0.getOpcode() == ISD::ADD) &&
----------------
efriedma wrote:
> Do you really need to explicitly use the number "64" to make this work?
Yes, it can be deleted


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

https://reviews.llvm.org/D126532