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

Fri May 6 02:17:09 PDT 2022

frasercrmck added a comment.

In D125021#3494767 <https://reviews.llvm.org/D125021#3494767>, @reames wrote:

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

It's certainly not enough to fix all cases, as I'm seeing new regressions when testing this internally. Looks like we might be missing coverage on loops. I'll get to work on reducing those failures and committing them. Your asserts might help with that.

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

Yeah I only really clocked this mutation thing yesterday. I thought that by including the "mutation" in `needVSETVLI` (now in phases 1 and 2) we might be able to get away with it being synced up?

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

Thanks. I do think having the one method to decide the compatibility and using it consistently across phases is logical and less error-prone. Unfortunately it seems we're not able to make what should be an NFC change without breaking things.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125021