[PATCH] D15124: Use @llvm.invariant.start/end intrinsics to extend the meaning of basic AA's pointsToConstantMemory(), for GVN-based load elimination purposes [Local objects only]

[Larisse Voufo via llvm-commits]

lvoufo marked 3 inline comments as done.
lvoufo added a comment.

In http://reviews.llvm.org/D15124#302677, @dnovillo wrote:

> I've just started looking at this, so these first few comments are mostly for my benefit to make sure I'm understanding this.
>
> Would it make sense to have tests that are purely around the InvariantInfo analysis?  This patch adds a user in AA and the test is on AA.  But if the analysis were to print its findings, we could have a test that simply checks that the analysis figures out what we are expecting without involving AA.


The problem with this approach is that the mapping does not get updated until instructions are traversed in -gvn; and by that time, -gvn already requires basic AA. Moreover, within the same pass, the mapping can change as invariant_start and invariant_end calls are encountered, adding or removing entries. The mappings are not fixed once updated like assumptions are for -assumption-cache-tracker.

With an updated (or rather non-empty) mapping, one could either print out analysis content to show that loads will be removed as expected (or not), or one could just go ahead and show that the said loads are removed. It is also not very clear to me what kind of information would be useful to print in this case, though I welcome any idea... Perhaps we could work on this in a separate patch/extension?

For the time being, it seems more effective to take the current approach. But perhaps I am missing something about LLVM's print analyses... I am also going from this notion that test cases are about confidence more than completeness. So, it is perfectly okay to center them around specific use cases showing the end results of the print messages (i.e. load removal) rather than the print messages themselves(?).


================
Comment at: include/llvm/Analysis/InvariantInfo.h:32
@@ +31,3 @@
+  /// GlobalVariable or AllocaInst instance) to its matching intrisic_start
+  /// call. This is to be used by GVN, InstCombine, GlobalOpt and BasicAA
+  /// to process invariant intrinsics for purposes of eliminating redundant
----------------
dnovillo wrote:
> Actually, this'd be used by *any* pass that needs to understand write-once objects, right?
Yes. This comment was specific to this particular design. But I could generalize it.

================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:687
@@ +686,3 @@
+static bool isInvariantIntrinsic(ImmutableCallSite CS) {
+  const IntrinsicInst *II = dyn_cast<IntrinsicInst>(CS.getInstruction());
+  return II && (II->getIntrinsicID() == Intrinsic::invariant_start ||
----------------
dnovillo wrote:
> Should this be defined in lib/Analysis/InvariantInfo.cpp?
Perhaps later, but for now, it's only used in this file?

================
Comment at: lib/Analysis/InvariantInfo.cpp:60
@@ +59,3 @@
+  //       So, do not insert nullptr values.
+  if (IStart) {
+    bool Inserted = InvariantMarkers.insert(std::make_pair(Addr, IStart)).second;
----------------
dnovillo wrote:
> Would it be better to have a remove() function in the interface, instead of making nullptr a magic value?
Yes.


http://reviews.llvm.org/D15124