[PATCH] D103510: [RISCV] Use vmv.v.v if we know COPY is under the same vl and vtype.

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.cpp:167
+      // If the vsetvli is tail undisturbed, keep the whole register move.
+      if (!RISCVVType::isTailAgnostic(VType))
+        return false;
----------------
What if the defining instruction something that doesn't obey vsetvli for example an earlier COPY that became a whole register move. Or a whole register load which could be a spill reload or just a VLMAX load.

Or a reduction which only writes element 0 regardless of the tail policy. I think we currently force tail agnostic on the vsetvli for those, but I might update it to keep whatever tail policy the previous instruction used.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.cpp:184
+        MachineOperand CopyAVL = VSetVLIForCopy->getOperand(1);
+        if (LMul == SetLMul && CopyLMul == SetLMul && CopySEW == SetSEW &&
+            ((CopyAVL.isReg() && SetAVL.isReg() &&
----------------
If the SEW and LMul are equal, isn't the vsetvli insertion pass usually going to use x0, x0 for VSetVLIForCopy?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.cpp:204
+        // 2 x LMUL.
+        if (LMul == SetLMul)
+          return true;
----------------
Is this equivalent to "return LMul == SetLMul;"?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.cpp:209
+      }
+    } else if (MBBI->isInlineAsm() || MBBI->isCall()) {
+      assert(!FoundDef && "There must be no inline asm or calls between "
----------------
What about this

vsetvli
v0 = vectorop
v1 = vleff
copy v2, v0

The vleff may change VL, but if you replace the copy you need the VL from before the vsetvli. So I think you need to stop if you encountered a vleff.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.cpp:375
+    if (UseVMV_V_V && (DefMBBI->getOpcode() == VIOpc)) {
+      UseVMV_V_I = true;
+      Opc = VIOpc;
----------------
Is this supposed to be in this patch. It's not mentioned in the description.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103510