[PATCH] D130442: [RISCV] Peephole optimization to fold merge.vvm and unmasked intrinsics.

[Yeting Kuo via Phabricator via llvm-commits]

fakepaper56 added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:154
 
+  MadeChange |= doPeepholeMergeVVMFold();
+
----------------
reames wrote:
> Order of operations - it seems like this transform should probably be done in the prior loop.  In particular, we probably want to fold the mask into the prior instruction before doPeepholeMaskedRVV turns it into an unmasked variant.  
The patch is try to work on unmasked intrinsics and some unmasked intrinsics are generated by doPeepholeMaskedRVV. Take an example, `vp.add` with all-one mask need `doPeepholeMaskedRVV` to be transformed to unmasked intrinsic.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:2559
+    // Skip if True has merge operand.
+    if (RISCVII::hasMergeOp(TII->get(TrueOpc).TSFlags))
+      continue;
----------------
craig.topper wrote:
> reames wrote:
> > craig.topper wrote:
> > > reames wrote:
> > > > I believe you have a missing check here.  Don't we need the passthrough for True to be False?  Otherwise, it seems like the vmerge might be merging two unrelated values?
> > > > 
> > > > We could potentially phrase this as an eager demanded lane style push back of the mask which wouldn't require that check, but that doesn't seem consistent with your code below.  
> > > This is only handling the case for True being an unmasked, tail agnostic instruction. That's why it calls `lookupMaskedIntrinsicByUnmaskedTA` below.
> > Right, but that's insufficient.
> > 
> > Converting:
> > vadd v1, v2, v3
> > vmul v4, v5, v6
> > vmerge v7, v1, v4, v0
> > 
> > To:
> > vadd v7 v2, v3, v0
> > vmul v4, v5, v6
> > 
> > Doesn't seem like what we want?
> > 
> > There's a variant here which would be legal, but it doesn't seem to be the transform implemented here.  That would be:
> > vadd v7 v2, v3, v0
> > v0 = not v0
> > vmul v7, v5, v6, v0
> Note the earlier check that the Merge operand and False operand of the VMERGE are the same.
Yeah. I thought about the scenario during writing the patch, but I wanted to decrease the complexity of the patch and I will do those work in the future. Maybe I could add some `TODO` in the comments?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130442