[PATCH] D125270: [riscv] Remove mutation of prior vsetvli from insertion dataflow

[Philip Reames via Phabricator via llvm-commits]

reames created this revision.
reames added reviewers: craig.topper, frasercrmck.
Herald added subscribers: sunshaoce, VincentWu, luke957, StephenFan, vkmr, evandro, luismarques, apazos, sameer.abuasal, s.egerton, Jim, benna, psnobl, jocewei, PkmX, the_o, brucehoult, MartinMosbeck, rogfer01, edward-jones, zzheng, jrtc27, kito-cheng, niosHD, sabuasal, bollu, simoncook, johnrusso, rbar, asb, hiraditya, arichardson, mcrosier.
Herald added a project: All.
reames requested review of this revision.
Herald added subscribers: pcwang-thead, eopXD, MaskRay.
Herald added a project: LLVM.

This fixes the last few cases where the state computed in phase 1 and 2 of the algorithm differs from the state computed during phase 3. Note that such differences can cause miscompiles by creating disagreements about contents of the VL and VTYPE registers at block boundaries.

There are two mutations being removed.  Both are buggy, but in slightly different ways.  Depending on how we count, we have maybe as many as three separate bugs here.

The "same AVL" mutation can cause the compatibility of a later store to change with regards to the current state.  test15 in the diff illustrates this case well.  What we have is a vsetvli which is mutated by one following vector op, but whose GPR result is used by another.  The compatibility logic walks back to the def in this case, and checks to see if it matches the immediate prior state.  In phase 1 and 2, it doesn't, and in phase 3 (after mutation) it does because we remove a transition which caused it to differ.

For the scalar move case, the existing transform is simply wrong.  We are correct about the fact that the scalar move itself can use the previous vsetvli, but we loose track of the fact that later instructions might depend on the state change represented.  This is annoying hard to demonstrate in practice due to differences in policy flags on the intrinsics, but this is at least a latent wrong code bug.

If we try to fix that issue by using the actual state of the mutated prior vsetvli (e.g. not the state produced by the instruction we skip producing), then a bunch of tests start either a) emitting redundant vsetvlis at the end of blocks due to the workaround, or b) failing asserts if 1) strict asserts are enabled, or 2) the block is a fallthrough block.

Unfortunately, the resulting patch is a bit ugly.  I'm really hoping a reviewer has an idea for a cleaner scheme here.  I've played with several, and this was the best of the lot.

This does cause some actual regressions as well.  I think the resulting code is a decent tradeoff between improvements and regressions, but some of the tests are worse off.  The major case is that we don't have good infrastructure for working with vsetvli x0, x0 <vtype> variants.  Unfortunately, we produce exactly that from the prior algorithm.

That ones a straight forward follow on at least.  A slightly trickier question is whether we should prefer LMAX or some constant for scalar moves?  The diffs are in all directions here, and we probably should pick one to canonicalize towards.  Its just not clear to me whether "vsetvili x0, 1, <vtype>" or "vsetvli not-x0, x0, <vtype> is better on the whole?  Particularly when we mix in surrounding code with other AVLs?


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D125270

Files:
  llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp
  llvm/test/CodeGen/RISCV/rvv/rv32-vsetvli-intrinsics.ll
  llvm/test/CodeGen/RISCV/rvv/rv64-vsetvli-intrinsics.ll
  llvm/test/CodeGen/RISCV/rvv/vsetvli-insert-crossbb.ll
  llvm/test/CodeGen/RISCV/rvv/vsetvli-insert.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D125270.428198.patch
Type: text/x-patch
Size: 12327 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220509/50a9b295/attachment.bin>