[PATCH] D103691: [RISCV] Add support for BITREVERSE for RVP

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:2025
     // Convert BSWAP/BITREVERSE to GREVI to enable GREVI combinining.
-    assert(Subtarget.hasStdExtZbp() && "Unexpected custom legalisation");
+    assert((Subtarget.hasStdExtZbp() || Subtarget.hasStdExtP()) &&
+           "Unexpected custom legalisation");
----------------
This will cause you to not assert for BSWAP when only RVP is enabled


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:2031-2034
+    // If RVP is enabled, lower BITREVERSE to customized BITREV node.
+    if (Subtarget.hasStdExtP() && Op.getOpcode() == ISD::BITREVERSE)
+      return DAG.getNode(RISCVISD::BITREV, DL, VT, Op.getOperand(0),
+                         DAG.getConstant(Imm, DL, VT));
----------------
Is BITREVI or bitmanip's REV better? Why do two extensions need to define the same instruction differently; can there not be a common sub-extension implemented by both (e.g. just the REV form of GREV), like how scalar crypto is using parts of bitmanip?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103691