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

[Luke Lau via Phabricator via llvm-commits]

luke marked an inline comment as done.
luke 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 =
----------------
craig.topper wrote:
> 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
Fixed in commit


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158161