[PATCH] D126921: [RISCV] Untangle instruction properties from VSETVLIInfo [NFC]

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added a comment.

I agree that instruction properties shouldn't be part of the abstract state, so I agree with the principle behind this patch.

I worry that this change is throwing another free variable into the mix when checking compatibility, which causes confusion and potentially leads to another sort of subtle bug - or people //worrying// about another sort of bug - about what happens if `MI` and `Require` are out of sync somehow.

As far as I can tell, `MI` and `Require` have to be in sync for correctness, is that right? We only ever call `needVSETVLI` when the `Require` has been computed from `MI`, anyway. So one idea I had was: may we have a tighter implementation if we removed the `Require` parameter from `needVSETVLI` and computed it fresh inside that function from `MI`? That way its API is obvious: "does this `MI` need a VSETVLI given the current VSETVLI state?". Now, we do need `Require` in other places outside of `needVSETVLI` so I dunno if it would just be moving things around as we'd be computing it twice - inside and out - rather than just the once.

Another option - could we assert that the `Require` is equivalent to that of a freshly-computed `VSETVLIInfo`?

Or we just document it in `needVSETVLI` and other APIs somehow. I see you've done that in `isCompatible` but I still worry about bugs if we're simply documenting rather than ensuring/checking/asserting.


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

https://reviews.llvm.org/D126921