[PATCH] D154245: [RISCV] Remove legacy TA/TU pseudo distinction for binary instructions

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.cpp:2543
                                   .add(MI.getOperand(0))
+                                  .addReg(MI.getOperand(0).getReg(), RegState::Undef)
                                   .add(MI.getOperand(1))
----------------
craig.topper wrote:
> What happens if we don't use undef here?
If I remember correctly, you get a verifier error due to a use before definition.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoVSDPatterns.td:729
 foreach vti = AllIntegerVectors in {
+  // The AddedComplexity here is covering up a missing matcher for
+  // widening vwsub.vx which can recognize a extended folded into the
----------------
craig.topper wrote:
> Add a FIXME here so we can find it later. Unless you're planning to fix it.
I'm not, so will do.


================
Comment at: llvm/test/CodeGen/RISCV/rvv/subregister-undef-early-clobber.mir:32
     dead $x0 = PseudoVSETIVLI 0, 210 /* e32, m4, ta, ma */, implicit-def $vl, implicit-def $vtype
-    early-clobber %0:vrm4 = PseudoVRGATHER_VI_M4 killed %6, 0, 0, 5 /* e32 */, implicit $vl, implicit $vtype
+    early-clobber %0:vrm4 = PseudoVRGATHER_VI_M4 undef %0, killed %6, 0, 0, 5/* e32 */, 0, implicit $vl, implicit $vtype
     %2:gpr = ADDI $x0, 0
----------------
craig.topper wrote:
> Should this be IMPLICIT_DEF instead? I see some other IMPLICIT_DEFs and no undefs in the existing code.
Looks like it yeah.  I got confused because this is post InsertVSETVLI.  Seeing implicit vtype and implicit vl almost always means your post regalloc, and I apparently didn't look close enough.  Update pending.


================
Comment at: llvm/test/CodeGen/RISCV/rvv/vxrm.mir:1
-# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 2
 # RUN: llc -mtriple=riscv64 -mattr=+v -verify-machineinstrs -stop-after prologepilog -o - %s | FileCheck %s --check-prefix=MIR
----------------
craig.topper wrote:
> Is this test no longer generated by the script?
>From what I could tell, it wasn't before.  It just had the header.  Or at least, auto-update didn't appear to work in any meaningful way.


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

https://reviews.llvm.org/D154245