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

[Nick Lewycky via llvm-commits]

nlewycky added a comment.

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.

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

-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.

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.

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.


================
Comment at: include/llvm/Analysis/Loads.h:41-43
@@ +40,5 @@
+/// value of '6'.
+#ifndef DEF_MAX_INSTS_TO_SCAN
+#define DEF_MAX_INSTS_TO_SCAN 6
+#endif
+
----------------
The usual way to do this in LLVM is "cl::opt". Take a look at UnrollCount for an example.

================
Comment at: include/llvm/Analysis/Loads.h:64
@@ -50,3 +63,3 @@
                                 BasicBlock::iterator &ScanFrom,
-                                unsigned MaxInstsToScan = 6,
+                                unsigned MaxInstsToScan = DEF_MAX_INSTS_TO_SCAN,
                                 AliasAnalysis *AA = nullptr,
----------------
Using a cl::opt means you won't get to have a default here (unless you pick a sigil value like -1 to indicate "use the default"). However, I think every caller of this function passes in 6 today, so the default argument is dead anyways.

However, I'm not sure why this should be a caller-controlled value at all.


http://reviews.llvm.org/D12886