[PATCH] D121881: [RISCV][RVV] Select unmasked TU RVV pseudos in a DAG post-process

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:2258
+        !N->getOperand(0).isUndef())
+      IsTU = true;
   }
----------------
arcbbb wrote:
> arcbbb wrote:
> > craig.topper wrote:
> > > khchen wrote:
> > > > frasercrmck wrote:
> > > > > If it's `TAIL_UNDISTURBED` and the merge operand //isn't// undef, shouldn't we be bailing?
> > > > I think we had not discuss what's proper way to handle conflict values between merge and policy operand.
> > > > 1. merge operand is not a undef value but policy is agnostic. (ex. tama)
> > > > 2. merge operand is a undef value but policy is undisturbed. (ex. tamu/tuma/tumu)
> > > > 
> > > > In fact, I didn't have strong opinion here. so in https://reviews.llvm.org/D120226 patch, I changed VSETVLI inserter to update `vtype` according to the policy value directly rather than checking merge operand is undef or not to do something (ex. report an error, or auto fixup), because I think users have explicitly specific the policy value, we need to respect it.
> > > > 
> > > > auto fixup means backend could update the policy value for 1. with `tumu` and 2. with `tama`. 
> > > > 
> > > > 
> > > > 
> > > From the register allocation standpoint, an undef operand gives the register allocator permission to pick whatever register is convenient. So it definitely won't preserve anything in a way a user could rely on. So in that case the policy is only going to affect the vsetlvli.
> > @frasercrmck Does this make sense to you?
> @frasercrmck Sorry for the noise. I didn't notice the enum was changed.
Sorry, I got a bit lost by the conversation and then it dropped off my radar. I think it's resolved though? Converting TU + IMPLICIT_DEF to TA makes sense to me.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td:429
   Pseudo UnmaskedPseudo = !cast<Pseudo>(!subst("_MASK", "", NAME));
+  Pseudo UnmaskedTuPseudo = !cast<Pseudo>(!subst("_MASK", "", NAME # "_TU"));
   bits<4> MaskOpIdx = MaskIdx;
----------------
arcbbb wrote:
> After D123217 there are cases where _TU pseudo doesn't exist.
> I have to fix this logic.
I'm sorry about that. Can we make it optional somehow?


================
Comment at: llvm/test/CodeGen/RISCV/rvv/allone-masked-to-unmasked.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=riscv64 -mattr=+v -verify-machineinstrs < %s | FileCheck %s
----------------
Could these tests be pre-commited?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121881