This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by luke on Aug 17 2023, 2:57 AM.

Details

Summary

When folding a vmerge into its operands, if the resulting VL is smaller than
what the vmerge had originally then what was previously in its body then gets
moved to the tail. In that case, we can't relax the tail policy to agnostic
when the merge operand is undefined, since we need to preserve these elements
past the new VL.

Fixes https://github.com/llvm/llvm-project/issues/64754

Diff Detail

Event Timeline

luke created this revision.Aug 17 2023, 2:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2023, 2:57 AM
luke requested review of this revision.Aug 17 2023, 2:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2023, 2:57 AM
reames added inline comments.Aug 17 2023, 11:38 AM
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
3512

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.

reames added inline comments.Aug 17 2023, 11:53 AM
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
3512

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.

luke added inline comments.Aug 18 2023, 3:06 AM
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
3512

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.

craig.topper accepted this revision.Aug 18 2023, 3:23 PM

LGTM, but maybe wait until @reames approves too

This revision is now accepted and ready to land.Aug 18 2023, 3:23 PM
craig.topper added inline comments.Aug 20 2023, 7:11 PM
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
3512

80 columns

reames accepted this revision.Aug 21 2023, 10:12 AM

LGTM. Sorry, should have done that Friday.

LGTM. Sorry, should have done that Friday.

I sat down and did the case analysis for all 9 combinations of policy states. We disallow certain combinations via early returns, and I believe that with this patch, all the other cases are covered correctly.

I think this might be a case where generalizing to handle the remaining cases actually simplifies and makes the code more obvious, but that's definitely a future patch!

This revision was landed with ongoing or failed builds.Aug 22 2023, 2:39 AM
This revision was automatically updated to reflect the committed changes.
luke marked an inline comment as done.Aug 22 2023, 2:40 AM
luke added inline comments.
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
3512

Fixed in commit