[RISCV] Attach an implicit source operand on vector copies

[mshockwave]

Somtimes when we're breaking up a large vector copy into several smaller ones, not every single smaller source registers are initialized at the time when the original COPY happens, and the verifier will not be pleased when seeing the smaller copies reading from an undef register. This patch is a workaround for the said issue by attaching an implicit read of the source operand on the newly generated copies.

This is tested by llvm/test/CodeGen/RISCV/rvv/vector-interleave.ll which would have crashed the compiler without this fix when LLVM_EXPENSIVE_CHECK is enabled. Original context: #124825 (comment)

[llvmbot]

@llvm/pr-subscribers-backend-risc-v

Author: Min-Yih Hsu (mshockwave)

Changes

Somtimes when we're breaking up a large vector copy into several smaller ones, not every single smaller source registers are initialized at the time when the original COPY happens, and the verifier will not be pleased when seeing the smaller copies reading from an undef register. This patch is a workaround for the said issue by attaching an implicit read of the source operand on the newly generated copies.

This is tested by llvm/test/CodeGen/RISCV/rvv/vector-interleave.ll which would have crashed the compiler without this fix when LLVM_EXPENSIVE_CHECK is enabled. Original context: #124825 (comment)

---

Full diff: https://github.com/llvm/llvm-project/pull/126155.diff

1 Files Affected:

• (modified) llvm/lib/Target/RISCV/RISCVInstrInfo.cpp (+5)

diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp b/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
index 12a7af07508136..47d6a5f4f95dfe 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
@@ -437,6 +437,11 @@ void RISCVInstrInfo::copyPhysRegVector(
       MIB.addReg(RISCV::VL, RegState::Implicit);
       MIB.addReg(RISCV::VTYPE, RegState::Implicit);
     }
+    // Add an implicit read of the original source to silence the verifier
+    // in the cases where some of the smaller VRs we're copying from might be
+    // undef, caused by the fact that the original, larger source VR might not
+    // be fully initialized at the time this COPY happens.
+    MIB.addReg(SrcReg, RegState::Implicit);
 
     // If we are copying reversely, we should decrease the encoding.
     SrcEncoding += (ReversedCopy ? -NumCopied : NumCopied);

[github-actions]

⚠️ undef deprecator found issues in your code. ⚠️

You can test this locally with the following command:

git diff -U0 --pickaxe-regex -S '([^a-zA-Z0-9#_-]undef[^a-zA-Z0-9_-]|UndefValue::get)' dcb124e820b2bf9dda60f66151591155a385580e aa8e9acd645d48fb971825f42a913a275c016b2c llvm/lib/Target/RISCV/RISCVInstrInfo.cpp

The following files introduce new uses of undef:

• llvm/lib/Target/RISCV/RISCVInstrInfo.cpp

Undef is now deprecated and should only be used in the rare cases where no replacement is possible. For example, a load of uninitialized memory yields undef. You should use poison values for placeholders instead.

In tests, avoid using undef and having tests that trigger undefined behavior. If you need an operand with some unimportant value, you can add a new argument to the function and use that instead.

For example, this is considered a bad practice:

define void @fn() {
  ...
  br i1 undef, ...
}

Please use the following instead:

define void @fn(i1 %cond) {
  ...
  br i1 %cond, ...
}

Please refer to the Undefined Behavior Manual for more information.

[wangpc-pp]

At what time the COPY happens? Why doesn't InitUndef work at this time?

[mshockwave]

The case I saw was created by register coalescer where it generates something like this:

%8.sub_vrm2_0:vrn32 = ...
%15.sub_vrm1_0_sub_vrm1_1_sub_vrm1_2_sub_vrm1_3_sub_vrm1_4:vrn3m2 = COPY %8.sub_vrm1_0_sub_vrm1_1_sub_vrm1_2_sub_vrm1_3_sub_vrm1_4

what happened was that the first instruction only initializes part of %8 and the second one COPY from another partially initialized (or partially undef) sub-register of %8. In this case we can't mark the source operand of COPY with undef. This COPY will eventually got broken down into smaller copies, in which one of those copies were copied from an undef register, hence the verifier error.

An alternative solution would be adding undef on source operand of the lowered, smaller copy instructions when needed. But that requires LiveIntervalsAnaysis.

[arsenm]

MIR test would be helpful, it's a huge pain to get this right and you need to consider kill and undef flags

[mshockwave]

MIR test would be helpful, it's a huge pain to get this right and you need to consider kill and undef flags

Done

[wangpc-pp]

LGTM. I don't see any concern.

[arsenm]

Should set tracksRegLiveness

[arsenm]

Test undef and kill flags