[PATCH] D126040: [InstCombine] Fold a mul with bool value into and

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In D126040#3530339 <https://reviews.llvm.org/D126040#3530339>, @nikic wrote:

> In D126040#3529149 <https://reviews.llvm.org/D126040#3529149>, @xbolva00 wrote:
>
>> Botan AES-128 benchmark, maybe others..
>
> As far as I can tell, this wouldn't help Botan AES-128, because it needs the variant where only one of the operands is 0/1. That does look more generally useful, but is also somewhat unclear from a canonicalization perspective, because it increases instruction count. (We'd probably want to represent it as `trunc(X) ? Y : 0` rather than `-X & Y`, but it's increasing the count either way.)

Yes, note that we do have these related folds already:

  // (zext bool X) * Y --> X ? Y : 0
  // (lshr X, 31) * Y --> (ashr X, 31) & Y

With these comments on the 2nd fold:

  // TODO: We are not checking one-use because the elimination of the multiply
  //       is better for analysis?
  // TODO: Should we canonicalize to '(X < 0) ? Y : 0' instead? That would be
  //       more similar to what we're doing above.

If we really want this fold in IR using known bits, then it could be implemented to reduce cost in 2 ways:

1. Check for 'nuw nsw' - if we know all of the high bits of both values are clear, then no-wrap should be set (and if they are not set, then there's no chance that computing known bits again is going to work here).
2. Predicate the 2nd call on the 1st instead of trying both before the 'if'.

But given that we already have the zext/shift patterns covered, maybe we just want to add the specific match for `and 1` as this patch was originally written. None of the tests are covering other patterns because we already get all of those?


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

https://reviews.llvm.org/D126040