[PATCH] D125392: [riscv] Canonicalize vsetvli (vsetvli avl, vtype1) vtype2 transitions

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:1170
+              // WARNING: For correctness, it is essential the contents of VL
+              // and VTYPE stay the same after this instruction.  This
+              // greatly limits the mutation we can legally do here.
----------------
frasercrmck wrote:
> reames wrote:
> > frasercrmck wrote:
> > > I think I'm getting confused by the comment here - what's "this instruction"? The current MI or the previous VSETVLI?
> > 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)
> 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.
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;
             }
           }
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125392