[PATCH] D103334: [GISel] Eliminate redundant bitmasking

[Jon Roelofs via Phabricator via llvm-commits]

jroelofs added inline comments.


================
Comment at: llvm/include/llvm/Target/GlobalISel/Combine.td:650
   redundant_and, redundant_sext_inreg, redundant_or, urem_pow2_to_mask,
-  zext_trunc_fold, icmp_to_true_false_known_bits]>;
+  zext_trunc_fold, icmp_to_true_false_known_bits, overlapping_and]>;
 
----------------
paquette wrote:
> Probably shouldn't be in this group since it doesn't use KnownBits?
Oops, yeah. I thought I was going to need KnownBits, but didn't end up using it.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:3008
+
+  // FIXME: This should be removed once GISelKnownBits supports vectors.
+  if (Ty.isVector())
----------------
paquette wrote:
> This doesn't use KnownBits?
> 
> (If this was yoinked from somewhere else, then we probably need to update a combine. GISelKnownBits supports vectors nowadays last time I checked.)
https://github.com/llvm/llvm-project/blob/8a5f0d883832aa088460df87df99c40d8f238655/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp#L3024


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:3025
+      B.buildCopy(Dst, B.buildConstant(Ty, 0));
+    }
+  };
----------------
paquette wrote:
> Minor stylistic suggestion:
> 
> ```
> if (C1 & C2) {
>   B.buildAnd(...);
>   return;
> }
> 
> B.buildCopy(...);
> ```
> 
> Is it actually necessary to emit the copy at all though? You could do
> 
> ```
> Dst = G_CONSTANT iN 0
> ```
> 
> right?
Are we guaranteed that `Dst` is not a physical register? I wasn't sure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103334