[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 inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2620
 
   // fold a+vscale(c1)+vscale(c2) -> a+vscale(c1+c2)
   if ((N0.getOpcode() == ISD::ADD) &&
----------------
With Further debuging , I found this pattern doen't match during llvm::BeforeLegalizeTypes because when the node **t26** is updated, it only add its user  node **t9** to  the Worklist, without its recursive user node **t10**
https://github.com/llvm/llvm-project/blob/main/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp#L1607

Surely, It's unreasonable to recursively add nodes **t10** to the worklist, as it may lead to a significant increase in compile time.

```
SelectionDAG has 16 nodes:
  t0: ch = EntryToken
        t2: i32,ch = CopyFromReg t0, Register:i32 %0
        t26: i32 = vscale Constant:i32<8>  --- update last
      t9: i32 = add nuw t2, t26
      t19: i32 = vscale Constant:i32<2>
    t10: i32 = add nuw t9, t19
  t12: ch,glue = CopyToReg t0, Register:i32 $w0, t10
    t4: i32 = vscale Constant:i32<1>
  t24: i32 = shl t4, Constant:i64<3>
  t13: ch = AArch64ISD::RET_FLAG t12, Register:i32 $w0, t12:1
```


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2628-2629
 
+  // fold a+truncate(vscale(c1))+truncate(vscale(c2))
+  // to a+truncate(vscale(c1+c2))
+  if (VT.isScalarInteger() && (N0.getOpcode() == ISD::ADD) &&
----------------
paulwalker-arm wrote:
> Would the following combine be safe?
>  ```
> ty1 truncate(ty2 vscale(c1)) -> ty1 vscale(c1)
> ```
> I ask because then the combine just above the new one would just work?
> 
> I guess the problem is that operation legalisation might be the thing introducing the truncate but then we can just limit the combine to before then.  I wouldn't expect the combine to be all that useful after legalisation anyway, although am happy to be proven wrong if you've a test case.
thanks @paulwalker-arm for your idea,  I thinks **ty1 truncate(ty2 vscale(c1)) -> ty1 vscale(c1)** is safe as C1 is a const. 
But If we combine this, the ISel may crash as we don't defined such pattern.

Yes, the truncate is introduced in AArch64TargetLowering::LowerVSCALE, where addressing the legalisation for the new added case **combine_add_vscale_C_i32**.  

BTW: Added the check Level > AfterLegalizeVectorOps according the comment


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2630
+  // to a+truncate(vscale(c1+c2))
+  if (VT.isScalarInteger() && (N0.getOpcode() == ISD::ADD) &&
+      (N0.getOperand(1).getOpcode() == ISD::TRUNCATE) &&
----------------
paulwalker-arm wrote:
> Is the `VT.isScalarInteger()` check necessary? I figure the later `ISD::VSCALE` requirement will guaranteed such, plus I don't see anything in the if block that actually cares.
Thanks, apply your comment, and deleted the unnecessary check.


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

https://reviews.llvm.org/D126532