[PATCH] D155071: [RISCV] Fold vmerge into its ops with smaller VL if known

[Luke Lau via Phabricator via llvm-commits]

luke marked 2 inline comments as not done.
luke added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:3333
+
+  // Because N and True must have the same merge operand, the "effective" body
+  // is the minimum of their VLs. For example, if we have VL=3 and VL=5:
----------------
luke wrote:
> luke wrote:
> > reames wrote:
> > > Nit pick - have the same merge operand *or the merge on the True instruction doesn't exist and is thus undef*.
> > My mental model of this is that we need to have the same merge operand for both, but if the merge operand on True is undefined or missing, then we assume it's the same merge operand as N. Could we make this reasoning more explicit in the preceding code?
> Something like:
> 
> ```
> diff --git a/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp b/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
> index d956b303e584..6a084efa0a8f 100644
> --- a/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
> +++ b/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
> @@ -3245,18 +3245,20 @@ bool RISCVDAGToDAGISel::performCombineVMergeAndVOps(SDNode *N) {
>    if (!Info)
>      return false;
>  
> -  if (HasTiedDest && !isImplicitDef(True->getOperand(0))) {
> -    // The vmerge instruction must be TU.
> -    // FIXME: This could be relaxed, but we need to handle the policy for the
> -    // resulting op correctly.
> -    if (isImplicitDef(Merge))
> -      return false;
> -    SDValue MergeOpTrue = True->getOperand(0);
> -    // Both the vmerge instruction and the True instruction must have the same
> -    // merge operand.
> -    if (False != MergeOpTrue)
> -      return false;
> -  }
> +  SDValue TrueMerge = HasTiedDest ? True->getOperand(0) : False;
> +  if (isImplicitDef(TrueMerge))
> +    TrueMerge = False;
> +
> +  // Both the vmerge instruction and the True instruction must have the same
> +  // merge operand.
> +  if (TrueMerge != False)
> +    return false;
> +
> +  // The vmerge instruction must be TU.
> +  // FIXME: This could be relaxed, but we need to handle the policy for the
> +  // resulting op correctly.
> +  if (isImplicitDef(Merge) && !isImplicitDef(TrueMerge))
> +    return false;
>  
>    if (IsMasked) {
>      assert(HasTiedDest && "Expected tied dest");
> ```
> 
@reames Do you have any thoughts on the above?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155071