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

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.

Generally seems correct, but enough minor comments that I want to see a revision before giving an LGTM.



================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:3319
+      return LHS;
+    if (auto *CLHS = dyn_cast<ConstantSDNode>(LHS),
+        *CRHS = dyn_cast<ConstantSDNode>(RHS);
----------------
luke wrote:
> fakepaper56 wrote:
> > Is the below code more concise?
> > ```
> > if (auto *CLHS = dyn_cast<ConstantSDNode>(LHS))
> >   if (auto *CRHS = dyn_cast<ConstantSDNode>(RHS))
> > ```
> Yeah that's much more readable, thanks!
Suggestion:

```
auto *CLHS = dyn_cast<ConstantSDNode>(LHS);
auto *CRHS = dyn_cast<ConstantSDNode>(RHS);
if (!CRHS || !CLHS)
  return SDValue();
return CLHS->getZExtValue() <= CRHS->getZExtValue() ? LHS : RHS;

```


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:3320
 
-  // We need the VLs to be the same. But if True has a VL of VLMAX then we can
-  // go ahead and use N's VL because we know it will be smaller, so any tail
-  // elements in the result will be from Merge.
-  if (TrueVL != VL && !isAllOnesConstant(TrueVL))
+  auto GetSmallerOrEqualVL = [](SDValue LHS, SDValue RHS) {
+    if (LHS == RHS)
----------------
Naming: GetMinVL


================
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:
----------------
Nit pick - have the same merge operand *or the merge on the True instruction doesn't exist and is thus undef*.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:3334
+  // 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:
+  //
----------------
This comment is awfully verbose, and I don't think the example helps much.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:3350
   // If we end up changing the VL or mask of True, then we need to make sure it
-  // doesn't raise any observable fp exceptions, since changing the active
-  // elements will affect how fflags is set.
-  if (TrueVL != VL || !IsMasked)
+  // doesn't raise any fp exceptions, since changing the active elements will
+  // affect how fflags is set.
----------------
The comment change needs reverted.


================
Comment at: llvm/test/CodeGen/RISCV/rvv/fixed-vectors-vselect.ll:250
 ; RV64-NEXT:    vsetivli zero, 6, e32, m2, ta, ma
 ; RV64-NEXT:    lbu a2, 0(a2)
+; RV64-NEXT:    vle32.v v8, (a1)
----------------
Totally off topic for this review, but we can do way better for odd sized mask loads.  The scalarization code here already touches all bits in the byte, we can just do a single byte vector load and mask off the high bits directly.  

Not sure we care, just noticed it as I was looking at this test.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155071