This is an archive of the discontinued LLVM Phabricator instance.

[riscv] Canonicalize vsetvli (vsetvli avl, vtype1) vtype2 transitions
ClosedPublic

Authored by reames on May 11 2022, 8:19 AM.

Details

Summary

This patch is an alternative to a piece of D125270. If we have one vsetvli which is using as AVL the output of another, and the prior AVL can be proven to produce the same VL value as that defining one, we can use the AVL from the prior instruction. This has the effect of removing a state transition on AVL, and will let us use the cheaper 'vsetvli x0, x0, vtype1' form or possible even skip emitting it entirely.

This builds on the same infrastructure as D125337, and does the analogous extension to working on abstract states instead of only prior explicit vsetvli instructions. This is where the (relatively minor) code improvements come from.

More importantly, this fixes the last case where the state computed in phase 1 and 2 of the algorithm differs from the state computed during phase 3. Note that such differences can cause miscompiles by creating disagreements about contents of the VL and VTYPE registers at block boundaries.

Doing this transform inside the dataflow can cause the compatibility of a later store to change with regards to the current state. test15 in the diff illustrates this case well. What we have is a vsetvli which is mutated by one following vector op, but whose GPR result is used by another. The compatibility logic walks back to the def in this case, and checks to see if it matches the immediate prior state. In phase 1 and 2, it doesn't, and in phase 3 (after mutation) it does because we remove a transition which caused it to differ.

Diff Detail

Event Timeline

reames created this revision.May 11 2022, 8:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2022, 8:19 AM
reames requested review of this revision.May 11 2022, 8:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2022, 8:19 AM
frasercrmck added inline comments.May 11 2022, 10:05 AM
llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
1165

Makes sense to me to do a post-pass. Though not for this patch.

1170

I think I'm getting confused by the comment here - what's "this instruction"? The current MI or the previous VSETVLI?

1258

possibly

reames added inline comments.May 11 2022, 10:18 AM
llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
1170

The current one. Essentially, if we skip emitting the state change, the mutated state on the prior MI (which we know is compatible with all of its own uses since we didn't change VL), must match what this state change would have produced. That is, the VL/VTYPE state after the the vector op must be the same, even if that vector op doesn't care. (e.g. we can't reintroduce the scalar move bug)

frasercrmck accepted this revision.May 11 2022, 10:30 AM

LGTM

llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
1170

Makes sense, thanks. I think it's just that the use this is somewhat confusing, since the comment is directly before a mutation of an instruction which can reasonably be thought of as "this" - and the comment doesn't make sense since it's obviously changing VTYPE. I think it might be better to explicitly name MI rather than relying on demonstrative pronouns.

This revision is now accepted and ready to land.May 11 2022, 10:30 AM
This revision was landed with ongoing or failed builds.May 11 2022, 10:45 AM
This revision was automatically updated to reflect the committed changes.
reames added inline comments.May 11 2022, 10:53 AM
llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
1170

Thinking about this, I think I can make this a bit more explicit in code. What would you think of the following as a follow on tweak?

iff --git a/llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp b/llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
index b0b911572506..df79dfbfa5b2 100644
--- a/llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
+++ b/llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
@@ -1170,6 +1170,10 @@ void RISCVInsertVSETVLI::emitVSETVLIs(MachineBasicBlock &MBB) {
               // and VTYPE stay the same after MI.  This greatly limits the
               // mutation we can legally do here.
               PrevVSETVLIMI->getOperand(2).setImm(NewInfo.encodeVTYPE());
+              // Keep the abstract state in sync with the register values
+              // (At the moment, this is only used for the following assert.)
+              CurInfo.setVTYPE(NewInfo.encodeVTYPE());
+              assert(NewInfo == CurInfo && "states out of sync!");
               NeedInsertVSETVLI = false;
             }
           }