[PATCH] D152937: [RISCV] Document overview of vector psuedos [nfc]

[Yueh-Ting (eop) Chen via Phabricator via llvm-commits]

eopXD added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.h:323
+  from this representation.
+  * _TU - Can represent all three policy states.  If passthrough is
+  IMPLICIT_DEF, then represents "undefined".  Otherwise, policy operand
----------------
luke wrote:
> eopXD wrote:
> > luke wrote:
> > > luke wrote:
> > > > Not for this patch, but should we standardise the terminology of passthrough vs. merge throughout the backend and pick one? My weak preference would be for passthrough as it’s less confusing with vmerge etc. 
> > > Not all TU pseudos have a policy operand (as of today). Should we document that without the policy operand it defaults to TUMU?
> > I think all unmasked Pseudo does not have a policy operand.
> > 
> > When the `merge` operand is undef, the tail policy is agnostic. When the `merge` operand is not undef, the tail policy is undisturbed.
> > 
> > I don't quite understand what are the instructions are affected by the default setting you have mentioned. My understanding is that at Pseudo instruction level the vector instructions all have explicit policy information. May you elaborate more?
> Thanks for clarifying this, I just did a quick check: with the exception of  the VPseudoTiedBinaryNoMask class, unmasked pseudos **don't** have a policy operand. 
> 
> The bit that I'm getting confused about is the sentence below:
> > Otherwise, policy operand and tablegen flags drive the interpretation.
> 
> Since TU pseudos are unmasked and don't have a policy operand.
> 
> By "default" I meant what policy is computed in `computeInfoForInstr` in `RISCVInsertVSETVLI.cpp` when there's no policy operand, and this is the piece of logic here:
> 
> ```
>   bool TailAgnostic = true;
>   bool MaskAgnostic = true;
>   if (!hasUndefinedMergeOp(MI, *MRI)) {
>     // Start with undisturbed.
>     TailAgnostic = false;
>     MaskAgnostic = false;
> 
>     // If there is a policy operand, use it.
>     if (RISCVII::hasVecPolicyOp(TSFlags)) {
>       const MachineOperand &Op = MI.getOperand(MI.getNumExplicitOperands() - 1);
>       uint64_t Policy = Op.getImm();
>       assert(Policy <= (RISCVII::TAIL_AGNOSTIC | RISCVII::MASK_AGNOSTIC) &&
>              "Invalid Policy Value");
>       TailAgnostic = Policy & RISCVII::TAIL_AGNOSTIC;
>       MaskAgnostic = Policy & RISCVII::MASK_AGNOSTIC;
>     }
> 
>     // Some pseudo instructions force a tail agnostic policy despite having a
>     // tied def.
>     if (RISCVII::doesForceTailAgnostic(TSFlags))
>       TailAgnostic = true;
> 
>     if (!RISCVII::usesMaskPolicy(TSFlags))
>       MaskAgnostic = true;
>   }
> ```
> 
> So as you say, unmasked pseudos don't have a policy operand and so are TUMU "by default", unless the merge operand is undefined in which case it's TAMA.
I think having undisturbed as default may not be a good choice, since the vector extension is targeting high performance, and possibly out-of-order cores. The undisturbed policy is likely to ruin the performance.



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

https://reviews.llvm.org/D152937