[PATCH] D50854: [LICM] Add a diagnostic analysis for identifying alias information of loads within loop

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.

Detailed comments inline.

A couple of testing notes:

1. don't the guard tests change output as well?  Or has Max's fix for that already landed?
2. we need some tests for readonly calls which are not guards or invariant.starts.



================
Comment at: lib/Transforms/Scalar/LICM.cpp:1588
 ///
 static bool pointerInvalidatedByLoop(Value *V, uint64_t Size,
                                      const AAMDNodes &AAInfo,
----------------
I don't see any reason not to merge these two functions.  They can both work on memory locations.  The internal behaviour can be conditional.  This also extends this to read only calls.  

Unrelated, but it just occurred to me this interface could be simplified with MemoryLocation.  Expect to have to rebase shortly.  :)


================
Comment at: lib/Transforms/Scalar/LICM.cpp:1597
+// instead of the cross product using AA.
+static cl::opt<int>
+LICMN2Theshold("licm-n2-threshold", cl::Hidden, cl::init(0),
----------------
Move to top of file


================
Comment at: lib/Transforms/Scalar/LICM.cpp:1601
+
+// The alias set mechanism used by LICM has a major weakness in that
+// it combines all things which may alias into a single set *before* asking
----------------
Comment doesn't seem consistent with this being only a diagnostic mechanism.  I think you need to adjust either your submission comment or this comment to be consistent.  

(For those reading along, this is a port of a downstream patch.  The primary *upstream* value is for finding AST deficiencies, but we do run with a small non-zero value for this.)


================
Comment at: lib/Transforms/Scalar/LICM.cpp:1634
+      N++;
+      auto Res = AA->getModRefInfo(
+          &I, MemoryLocation(LI->getPointerOperand(), Size, AAInfo));
----------------
You can lift the formation of the MemoryLocation out of this loop, and even out of the function.


Repository:
  rL LLVM

https://reviews.llvm.org/D50854