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

[Philip Reames via Phabricator via llvm-commits]

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

Have you looked at doing this as a dag combine instead of a post-isel peephole?  I believe we should have SDNodes with masks for all of these, doing the local peephole in a combine and letting it iterate seems more straight forward.  (I'm probably missing something, right?)



================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:154
 
+  MadeChange |= doPeepholeMergeVVMFold();
+
----------------
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.  


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:2530
+    unsigned Opc = N->getMachineOpcode();
+    // FIXME: Also deal with PseudoVMERGE_VVM_<LMUL>
+    if (Opc != RISCV::PseudoVMERGE_VVM_MF8_TU &&
----------------
Extract out a helper function for the opcode matching please.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:2559
+    // Skip if True has merge operand.
+    if (RISCVII::hasMergeOp(TII->get(TrueOpc).TSFlags))
+      continue;
----------------
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.  


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:2608
+    // Try to transform Result to unmasked intrinsic.
+    doPeepholeMaskedRVV(Result);
+    MadeChange = true;
----------------
This doesn't make any sense.  The only way this could succeed is if the mask was all ones; we shouldn't have a merge with an all ones mask to begin with right?


================
Comment at: llvm/test/CodeGen/RISCV/rvv/rvv-peephole-vmerge-vops.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -mtriple=riscv64 -mattr=+v | FileCheck %s
----------------
Can you precommit these tests?

Please also add tests for fixed-length vectors with explicit vector length set on the runline.   I remember looking at this a bit before and convincing myself there were some complicating issues with fixed for this, but not what those were unfortunately.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130442