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

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck accepted this revision.
frasercrmck added a comment.
This revision is now accepted and ready to land.

LGTM



================
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.
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125392