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

[Noah Goldstein via Phabricator via llvm-commits]

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:
> goldstein.w.n wrote:
> > 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.
> ping @nikic, okay with as is or okay with regressions or something else?
ping2 @nikic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146350