[PATCH] D124833: [RISCV] Optimize redundant vsetvli for Vector Mask-Register Logical Instructions.

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.

Sorry for not seeing/ignoring this for so long.

Rebase needed.

The basic structure of your patch makes sense, but can I suggest that you split this into two patches.

The first isn't specific to logical mask ops at all.  It would handle instructions with fixed policy bits generically.  These instructions don't have policy ops, which means the existing computeInfoForInstr code which just pick some default.  I think you can write a generic change which uses usesMaskPolicy and doesForceTailAgnostic to allow the tail policy difference in needVSETVLI.

The second patch is basically this one, but with clear comments about the VL interaction and reusing the generic bit from patch one for the policy handling.



================
Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:733
+
+  if (CurInfo.getSEWLMULRatio() == Require.getSEWLMULRatio())
+    return true;
----------------
Unless I'm missing something here, the vector mask logical instructions (15.1) do depend on VL.

Reading through that section, the handling of SEW/LMUL is a bit unclear.  I think what it's trying to say is that the value of VL is always such that <VL x i1> is known to fit within one vector register, and thus that the register operands refer to the actual registers, not the register groups.  However, it's not clear to me if SEW has any meaning here.  I think it probably doesn't?

Can you clearly state what you think the logical mask instructions do here?  Ideally, with pointers to the spec backing that?


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:1115
         if (needVSETVLI(NewInfo, BlockInfo[MBB.getNumber()].Pred) &&
             needVSETVLIPHI(NewInfo, MBB)) {
           insertVSETVLI(MBB, MI, NewInfo, BlockInfo[MBB.getNumber()].Pred);
----------------
JFYI, the lack of the change here is suspicious.  We in fact had a bug here for load/store case.

If you simply rebase, and sink your change inside the new version of needVSETVLI which takes MI, you should be fine here though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124833