[PATCH] D158161: [RISCV] Don't relax policy to ta when vmerge's VL shrinks during folding
Luke Lau via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Aug 22 02:40:19 PDT 2023
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
More information about the llvm-commits
mailing list