[PATCH] D140371: [Attributor] potential constant values for PHI and Load

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds added a comment.

In D140371#4007378 <https://reviews.llvm.org/D140371#4007378>, @sameerds wrote:

> In D140371#4007004 <https://reviews.llvm.org/D140371#4007004>, @jdoerfert wrote:
>
>> LG.
>>
>> Can you maybe add a potential value test that checks we can fold `icmp eq %l, 15` if we know `%l = load ...` is either `14` or `16`. Just to get "more direct" coverage.
>
> I tried adding a test with a conditional branch and two stores that reach the same load. But the log shows that AAPotentialConstantValues is never created, and hence the icmp is never simplified. It's not clear to me about when is it that AAPotentialConstantValues is actually consulted by the Attributor. Perhaps getSingleValue() or getAssumedSimplified() needs to do that?

I have been looking at this a bit, and I am not sure why AAPotentialConstantValues needs to exist as a separate attribute. Most of its optimizations can be folded into AAPotentialValuesImpl::simplifyInstruction(). For example, an icmp can be optimized if any one operand or both are known to be undef, or when both operands are known to have constant potential values. This can be checked inside simplifyInstruction() by simply moving most of the internals there from AAPotentialConstantValues. If a client needs to know if all potential values are constants, that can be easily arranged without needing a separate attribute.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140371