[PATCH] D146350: [InstCombine] More aggressively try and fold irem/idiv/mul into selects.

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n marked 3 inline comments as done.
goldstein.w.n added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:206
+             isSafeToSpeculativelyExecute(&I, Q->CxtI, Q->AC, Q->DT, TLI))
+      return false;
+  }
----------------
goldstein.w.n wrote:
> nikic wrote:
> > nikic wrote:
> > > Is this code in here so the case where the div is always folded away is always handled, as we're at worst going to relax to poison there and not introduce UB?
> > > 
> > > If so, I think it might be more elegant to sink this check into FoldOpIntoSelect and use the isSafeToSpeculativelyExecute variant that accepts operands, so we can check the operation that we're actually going to introduce (and thus also skip the check if we're not going to introduce one).
> > > and use the isSafeToSpeculativelyExecute variant that accepts operands
> > 
> > Well, it looks like I imagined that variant... So ignore that bit (or leave a TODO for possible future improvement) and just take the bit about only checking it if we're going to introduce an op.
> Without that variant we end up with regressions in:
> 
> ```
>   LLVM :: Transforms/InstCombine/div.ll
>   LLVM :: Transforms/InstCombine/rem.ll
> ```
> 
> I'm happy to just add the variant and use that, but I'm not sure what to do with `load`/`call` instructions.
> Maybe just return false in those cases as they aren't really needed here?
There are regressions either way. Would prefer to keep it this way if thats okay with you.


================
Comment at: llvm/test/Transforms/InstCombine/binop-select.ll:548
   ret i32 %div
 }
----------------
nikic wrote:
> goldstein.w.n wrote:
> > nikic wrote:
> > > Also test the straightforward case where the mul has a constant operand? Or is that already handled somewhere else (and if so, can we remove it)?
> > hmm? Not sure what you mean by this comment.
> I mean something like `(c ? x : C1) * C2`, that isn't based on the select condition.
done although unaffected by this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146350