[PATCH] D68930: [InstCombine] Shift amount reassociation in shifty sign bit test (PR43595)

Wed Oct 30 08:53:03 PDT 2019

spatel added a comment.

In D68930#1720610 <https://reviews.llvm.org/D68930#1720610>, @lebedev.ri wrote:

> @spatel
>
> In D68930#1715726 <https://reviews.llvm.org/D68930#1715726>, @lebedev.ri wrote:
>
> > In D68930#1715721 <https://reviews.llvm.org/D68930#1715721>, @spatel wrote:
> >
> > > In D68930#1715680 <https://reviews.llvm.org/D68930#1715680>, @lebedev.ri wrote:
> > >
> > > >
> > >
> > >
> > > , and I agree it's out-of-scope to beat up on this patch alone, so I won't hold it up. But we should take a step back, do some measurements, and decide how things can be split up.
> >
> >
> > Any thoughts on the approach i suggested:
> >
> > > All that being said, if it's really this much of a concern to warrant immediate action,
> > >  can't we simply expose `"Depth"` in the `llvm::SimplifyAddInst()`
> > >  and call it with such a depth that either disallows recursive reasoning,
> > >  or severely limits it? Some fastpaths will likely needed to be added then most likely.
> > >  That may be the best outcome considering the options..
> >
> > ?
>
>
> Up

Still catching up on mail backlog...
Exposing/varying the depth param doesn't seem like the right way to approach this to me. My fear is that we'd end up with semi-random uses of caller depths that wouldn't age well. So we might handle all of the cases in *this* patch with depth=1, but then someone notices their code needs depth=2 to get the optimization and changes it. The depth param is just a hack based on a magic number, so it feels wrong to expand on that hack. I don't think we can distinguish compile-time cost at that level anyway (but it would be great to have data). Apart from all of that, messing with recursion depth doesn't address the underlying problem that the code is just too big/complicated.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68930