[PATCH] D69748: [Attributor][IPConstantProp] Run the Attributor on IPConstantProp tests

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

>> Also, unfortunately I did not have time to followd the attributor development closely lately, I am just curious: is there a benefit of doing some value simplifications in attributor directly?
> 
> TL;DR, I think so yes, at least in the short term. How we want to go forward once the Attributor matured fully is an open question.
> 
> There are reasons, but, as always, it is not a clear win. So perfect world, we have a single place with "IP-SCCP-like" logic that can make use of all "optimistic in-flight information" during a fixpoint iteration.
>  For now, we will have some IP-SCCP like logic rewritten in the Attributor framework, see for example D69605 <https://reviews.llvm.org/D69605> for liveness improvements. The conditional folding logic needs cleanup before I put them here but that should happen in the coming days as well. Now we want this because we want/need liveness information during the deduction and we can use other information that is deduced to improve the value simplification part (think constant propagation) and thereby liveness. One example would be capture information and memory access information based on liveness information which feeds back into the rest.

Would it be sufficient to have IPSCCP simplify the module before running the Attributor? Ideally it should apply the simplifications based on liveness (module cases it misses) and the attributor would not have to worry about them. I think it would be good try to avoid splitting development effort and keeping Attributor focused should simplify the way towards enabling it.

> As an actually unrelated example, this code (https://godbolt.org/z/vP-vvM) works with the Attributor while IP-SCCP for some reason dislikes the dead `unknown(&X)` call. In the Attributor framework, it doesn't matter because the use of X is simply never shown to the relevant AAValueSimplify class as long as we assume it is dead. (All `Attributor::checkForAllXXXX(Predicate, ...)` methods simply "hide" dead code. XXXX can be CallSites, ReturnValues, Uses,....)
> 
>   static int X = 0;
>   void unknown(int *);
>   
>   int foo(void) {
>     if (X == 1)
>       X = 2;  
>     if (X == 2)
>       unknown(&X);
>     return X;
>   }

This is interesting. I'll check what's going on with IPSCCP here. It might miss some global initializer facts.

With respect to the tests in this patch, I'd slightly prefer copying them to an attributor directory (this might avoid confusion on the future and makes it easier to ensure new tests added to the directory check the right thing), but it's also fine as is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69748