[PATCH] D12886: Clean up: Refactoring the hardcoded value of 6 for FindAvailableLoadedValue()'s parameter MaxInstsToScan.

[Larisse Voufo via llvm-commits]

lvoufo added a comment.

In http://reviews.llvm.org/D12886#246629, @nlewycky wrote:

> A few unconnected issues:
>
> -1-
>  Please generate the patch with either:
>
>   git diff -U999999 other-branch
>
> or
>
>   svn diff --diff-cmd=diff -x -U999999
>
> per the instructions at http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface . The symptom of not doing this is that I can't easily see the rest of the function around the parts you modified, and that makes it hard to review the code. I need to go find a copy elsewhere to read.


Ok. Done.

> -2-

>  Does "opt -basicaa -gvn" clean up the redundant load? If so, is it okay that instcombine doesn't?


It does not. Neither "-basicaa -gvn", nor "-basicaa -early-cse -gvn", "nor -basicaa -early-cse -gvn -instcombine", ...
the issue is that the load is actually eliminated with "-O1", but not with "-O2". Examining the passes in -O2, I was able to reduce the problem area down to "-basicaa -instcombine -inline -early-cse -instcombine" (on a lowered sample *.cc program, lowered with optimizations turned off).

> -3-

>  My first preference is to not have instcombine do this transform at all, but I have no suggestion for how to make that happen. The xform redundant with all of mem2reg, sroa, early-cse and gvn, but IIRC it exists to fix a pass ordering problem where one iteration of instcombine makes the transform possible, and doing this load removal makes a transform in the next iteration of instcombine possible.


That may be so, but regardless of why it is the way it is at the moment, how it is affects compilation with -O2.
What is -instcombine designed for in the first place?
W.r.t. load instructions, I view it as a place where duplicate loads are combined so long as they are now too far apart from one another.

> My second preference is to not rely on a threshold. It's subject to problems like this one, where minor changes that aren't relevant to the semantic behaviour of the program will change the code we generate. We shouldn't have those. One possibility is to track loads as we scan forwards, much like early-cse does. It's a fair amount of code, but actually quite cheap in compile time especially when compared to everything else instcombine does. The idea is that you load, you add it to a map, when you perform an action which may write memory (non-readonly call, or store) you wipe the map. This way, you never need to scan backwards.


A good starting point would be to a least understand the origin of the current semantics. What does '6' mean? Why '6'? Why not something else? Any documentation available?

> My third preference is to query a system designed to do this with caching of queries, such as MemoryDependenceAnalysis (still scans backwards) or MemorySSA (not ready yet). Yes, this makes Instcombine a real clone of the PRE half of GVN instead of just a half-way pretending piece that doesn't really do things right.

> 

> Fixing any of the above is a serious project that would take quite some time.

> 

> And finally we can bump 6 to 8.


Ok. Could do this temporarily, and explore more future-proof solutions afterwards.


http://reviews.llvm.org/D12886