[PATCH] D86364: [ValueTracking] Interpret GEPs as a series of adds multiplied by the related scaling factor

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.

Hi @nikic, @spatel

> If we remove that from the equation, I expect the compile-time impact of this patch would be pretty minimal. I started some work on this, but I'm not sure when I'll be able to finish it.

That's an interesting observation. It would fit my use case to have a different API for compute known bits for all vs. compute known bits for all but only alignment for geps.

That said, from the users stand point how would we decide which version we want to use. E.g., what would be the criteria for instcombine?

If we look at https://llvm.org/PR47241 for instance, instcombine would need the full range to be able to optimize that code, but maybe we don't care about this use case.
Similarly, if we look at the change in llvm/test/Transforms/InstCombine/constant-fold-address-space-pointer.ll, even if we are only interested in the alignment, the new computation can yield better results.

> It's been a couple of months, so we should confirm that the time impact is still the same.

Thanks @nikic for the new measurements!
Looks like it's smaller, but the benefits may still not be worth it in general.

Let me know what do you think.

> It was suggested earlier to trigger the analysis from a different/dedicated pass rather than instcombine. Is that not feasible?

Repeating what I said earlier in that message, for my use case, that would certainly work. The one difficulty is that it needs to be exposed as a full fledged version of compute knowns bits, because I don't know before hand if the expression dag I am looking at contains any gep (and in particular the root may not be a gep). So we have to somehow convey within compute known bits that we want to compute only `TrailZ` instead of the full information. I would rather that we avoid this kind of plumbing, especially because it would create a precedent for new improvements in compute known bits, which if we go down that road is not very far from D87342 <https://reviews.llvm.org/D87342>.

> We're using the more expensive analysis on all GEPs. Could we use the existing analysis as the common case and limit the more expensive some way (for example, only GEPs with N or more operands, inbounds, etc)?

I can give that a try and see how it impacts compile time. The cut-off would be somewhat arbitrary though.

Stay tuned.

Cheers,
-Quentin


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86364