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

[Philip Reames via Phabricator via llvm-commits]

reames 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
----------------
eopXD wrote:
> 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.
> 
I'm trying to move away from a model where the default is meaningful.  Always having an explicit policy operand means the instruction carries the flags, and we can simply setup an agnostic flag if useful.  


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.h:328
+  overlap constraint mentioned above.
+ */
+struct RISCVMaskedPseudoInfo {
----------------
craig.topper wrote:
> How do we represent "undefined" for fma, integer multiple-accumulate where the destination is also an arithmetic source. Also vslideup where the destination is used for elements below offset and for the tail, but only tail follows the tail policy.
> 
> How do we represent tail "undefined" vs tail "agnostic" for _MASK?
Right now, we don't represent undefined for either of these cases.  We could extend the policy operand with additional bits if desired, but for the moment, but I'm not sure this is warranted at this time.


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

https://reviews.llvm.org/D152937