[PATCH] D125021: [RISCV] Fix VSETVLI insertion by syncing phases 2 and 3

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Have you tried adding asserts to the end of emitVSETVLIs to ensure the final block state is as expected?  I strongly suspect this change is insufficient to fix all the cases.

I've been working on the same problem, but with a different approach.  I've been trying to separate the PHI case and the mutation case into a separate phase 4 which works over the fully materialized form.  I've got all the transforms (messily) implemented, but even with those out of line, I'm still getting failures of the expected block end state assert.  I don't yet know why.

One really nasty case I came across - that I don't think your change handles - is that mutation of the prior vsetvli can change whether the current state is compatible with a following instruction.  The compatibility logic includes a bit which looks back at the defining vsetvli for a vreg operand, and since we mutated it, the result can change between phase 1 and phase 3.  At the moment, I'm thinking we can't have both that compatibility rule and the mutation in use in the same pass.

Now despite all that, I think your changes here really help in terms of code structure.



================
Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:1050
+  BBInfo.Change = InInfo;
+  computeVLVTYPEChanges(MBB);
+
----------------
This bit on it's own is super important.  It's slower, but it avoids the whole need for the merge and Change state entirely.  

I think this is a good idea, and probably stands on it's own.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125021