[PATCH] D141204: [GlobalISel] Extending G_SBFX/G_UBFX combine to consider G_SEXT_INREG as well

Thu Jan 12 06:30:10 PST 2023

foad added a comment.

I wonder if a simpler approach would be just to combine the `ashr (sextinreg x, 8), 4` into `sextinreg (ashr x, 4), 4` (i.e. canonicalize ashr-of-sext into sext-of-ashr) and then let other combines simplify it further?

================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:4399

   // Try to match shr (shl x, c1), c2
+  if (!mi_match(
----------------
Update the comment to say what else you are trying to match.

================
Comment at: llvm/test/CodeGen/AArch64/GlobalISel/form-bitfield-extract-from-shr.mir:194
+    %2:_(s32) = G_CONSTANT i32 4
+    %3:_(s32) = COPY %1:_(s32)
+    %4:_(s32) = G_SHL %0:_, %3:_(s32)
----------------
I don't think you need any of these COPYs of G_CONSTANTs.

================
Comment at: llvm/test/CodeGen/AArch64/GlobalISel/form-bitfield-extract-from-shr.mir:211
+    ; CHECK-NEXT: [[C1:%[0-9]+]]:_(s32) = G_CONSTANT i32 1
+    ; CHECK-NEXT: [[UBFX:%[0-9]+]]:_(s32) = G_UBFX [[COPY]], [[C1]](s32), [[C]]
+    ; CHECK-NEXT: $w0 = COPY [[UBFX]](s32)
----------------
This just seems wrong. You are combining `(sextinreg ($w0 << 3), 8) >> 4` to `ubfx $w0, 1, 4`.

If $w0 was 0x000000FF then the original code gives 0x0FFFFFFF but the combined code gives 0x0000000F.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141204