[PATCH] D96661: [RISCV] Move SHFLI matching to DAG combine. Add 32-bit support for RV64

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added a comment.

I think there are also a few linter issues to fix up here and there.

In general, though, it looks good. I'll take a closer look soon when I find some time.



================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:2492
 
 // Matches any of the following bit-manipulation patterns:
 //   (and (shl x, 1), (0x55555555 << 1))
----------------
I wonder if this comment should be updated. It's more or less correct (for grev) but is now too specific.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:2523
     return None;
-
-  // These are the unshifted masks which we use to match bit-manipulation
-  // patterns. They may be shifted left in certain circumstances.
-  static const uint64_t BitmanipMasks[] = {
-      0x5555555555555555ULL, 0x3333333333333333ULL, 0x0F0F0F0F0F0F0F0FULL,
-      0x00FF00FF00FF00FFULL, 0x0000FFFF0000FFFFULL, 0x00000000FFFFFFFFULL,
-  };
-
-  unsigned MaskIdx = Log2_64(ShAmt);
-  if (MaskIdx >= array_lengthof(BitmanipMasks))
+  // If we don't have enough masks for 64 bit, then we must be trying to
+  // match SHFL so we're only allowed to shift 1/4 of the width.
----------------
This bit seems specific to SHFL in a roundabout way. Is there a way to make it more generic, or at least explicit? I know we're only matching GREV or SHFL so we can expect certain preconditions but I worry it'd catch someone out if they wanted to use this method for other purposes.


================
Comment at: llvm/test/CodeGen/RISCV/rv32Zbp.ll:2879
   %and1 = and i64 %shl, 4919131752989213764
-  %or = or i64 %and1, %and
+  %or = or i64 %and, %and1
   %shr = lshr i64 %a, 1
----------------
Are these changes in operand order creating cases that weren't previously optimized? I take it it's not now required for the optimization to work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96661