[PATCH] D116307: [RISCV] Teach VSETVLInsert to eliminate redundant vsetvli for vmv.s.x and vfmv.s.f.

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:259
+    // situation.
+    if (InstrInfo.ScalarMovOp && InstrInfo.hasAVLImm() &&
+        ((hasNonZeroAVL() && InstrInfo.hasNonZeroAVL()) ||
----------------
Add `!Strict` to this.


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:262
+         (hasZeroAVL() && InstrInfo.hasZeroAVL())) &&
+        hasSameSEW(InstrInfo) && hasSamePolicy(InstrInfo))
+      return true;
----------------
The LMUL does matter if the policy is tail agnostic. If the last vsetvli has a larger lmul than the vmv.s.x instruction, then by ignoring LMUL you gave hardware permission to replace every element except 0 of the larger lmul with all 1s. But the vmv.s.x was only allowed to replace the smaller lmul.

I think there's also an issue if the register isn't aligned to a multiple of the LMUL setting in vtype. For example, if LMUL=8, the vmv.s.x can't use v2 as its register. Section 3.4.2 of the V spec says that is reserved.

```
When LMUL=8, the vector register group contains eight vector registers, and instructions specifying an LMUL=8 vector register group using register numbers that are not multiples of eight are reserved.
```


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:159
 
+  bool hasSameSEW(const VSETVLIInfo &Other) const {
+    assert(isValid() && Other.isValid() &&
----------------
jacquesguan wrote:
> craig.topper wrote:
> > Is this used?
> Done, now use the SEW and policy instead of VTYPE in the condition, because these instructions ignore the LMUL.
Add this assert from hasSameVTYPE

```
    assert(!SEWLMULRatioOnly && !Other.SEWLMULRatioOnly &&
           "Can't compare when only LMUL/SEW ratio is valid.");
```


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:259
 
+    // For vmv.s.x and vfmv.s.f, there is only two behaves, VL = 0 and VL > 0.
+    // So it's compatible when we could make sure that all two VL be the same
----------------
craig.topper wrote:
>  behaves -> behaviors?
This comment was not addressed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116307