[PATCH] D158161: [RISCV] Don't relax policy to ta when vmerge's VL shrinks during folding

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:3512
+  bool MergeVLShrunk = VL != OrigVL;
+  uint64_t Policy = (isImplicitDef(Merge) && !MergeVLShrunk) ? RISCVII::TAIL_AGNOSTIC : /*TUMU*/ 0;
   SDValue PolicyOp =
----------------
luke wrote:
> reames wrote:
> > reames wrote:
> > > If instead, we were to change this line to:
> > > ```
> > > uint64_t Policy = isImplicitDef(False)  ? RISCVII::TAIL_AGNOSTIC : /*TUMU*/ 0;
> > > 
> > > ```
> > > What would happen?  I'm worried there's some other case we haven't caught where Merge was implicit_def, but the resulting operation has a non-implicit_def result.  I think *all* such cases needs to be TU.  
> > Answer: A lot of regressions.  I tried this, and it definitely doesn't seem to work.
> > 
> > I'm still a bit worried there's another case here.  I don't have anything specific though.
> > 
> > 
> The new tail of the Result node is every element from min(vmerge.vl, true.vl) onwards. 
> 
> Therefore if we want to set a tail agnostic policy on it, we need to know that every element of vmerge from min(vmerge.vl, true.vl) onwards would have been implicit-def anyway.
> 
> The vmerge pseudo gets its tail from Merge, so if the Merge at that element was implicit-def, then that tail element will be implicit-def.
> 
> implicit-def(Merge[i])
> ->
> implicit-def(vmerge[i]),  vmerge.vl <= i < vlmax
> 
> We can rewrite this to
> 
> implicit-def(Merge[i]) ∧ vmerge.vl == min(vmerge.vl, true.vl)
> ->
> implicit-def(vmerge[i]),  min(vmerge.vl, true.vl) <= i < vlmax
> 
> So as long as we check `isImplicitDef(Merge)` and `VL == OldVL`, then we can infer the result
> 
> implicit-def(vmerge[i]),  min(vmerge.vl, true.vl) <= i < vlmax
> 
> i.e. every element past min(vmerge.vl, true.vl) in vmerge would have been implicit-def.
> 
> Btw I tried substituting Merge for False as well before posting the patch and got the same result.
> 
> implicit(False) -> implicit(Merge) so as long as we also check the VL stays the same, it's a correct transformation, just not optimal.
80 columns


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158161