[PATCH] D14003: Express and handle specific info about the processing of invariant intrinsics

[Nick Lewycky via llvm-commits]

nlewycky added inline comments.

================
Comment at: include/llvm/IR/InvariantInfo.h:21-23
@@ +20,5 @@
+namespace llvm {
+class Value;
+class DataLayout;
+class IntrinsicInst;
+
----------------
Alphabetize.

================
Comment at: include/llvm/IR/InvariantInfo.h:25-27
@@ +24,5 @@
+
+/// PreservedInvariantInfo -- A data structure to mark and access processed
+/// invariant intrinsic calls.
+class InvariantInfo {
+
----------------
One side says "PreservedInvariantInfo" the other side says "InvariantInfo". Just remove the class name from the comment, that style is pre-style guide.

Also, "a data structure to mark and access processed invariant intrinsic calls"? I don't think that tells me what it does or what it's for. Is it a cache of invariant intrinsics that pertain to a given Value?

================
Comment at: include/llvm/IR/InvariantInfo.h:29
@@ +28,3 @@
+
+  /// \brief A mapping of each 'writeonce' allocated memory (via a
+  /// GlobalVariable or AllocaInst instance) to its matching intrisic_start
----------------
"writeonce" is not defined anywhere?

================
Comment at: include/llvm/IR/InvariantInfo.h:30
@@ +29,3 @@
+  /// \brief A mapping of each 'writeonce' allocated memory (via a
+  /// GlobalVariable or AllocaInst instance) to its matching intrisic_start
+  /// call. This is to be used by GVN, InstCombine, GlobalOpt and BasicAA
----------------
Typo in "intrisic_start"

================
Comment at: include/llvm/IR/InvariantInfo.h:46
@@ +45,3 @@
+
+/// PreservedInvariantInfo -- Information processed from invariant intrinsics,
+/// for a given query instruction.
----------------
Remove struct name from comment.

================
Comment at: include/llvm/IR/InvariantInfo.h:58
@@ +57,3 @@
+/// from processed invariant intrinsics.
+class PreserveInvariantInfo {
+  PreservedInvariantInfo Preserved;
----------------
Hold on, you have one struct PreservedInvariantInfo and one class PreserveInvariantInfo? Please rename one of them.

================
Comment at: include/llvm/IR/Module.h:177
@@ -175,2 +176,3 @@
   DataLayout DL;                  ///< DataLayout associated with the module
+  InvariantInfo InvInfo;          ///< Processed invariant intrinsics info.
 
----------------
A Module has a 1:1 correlation with the .ll and the .bc. This shouldn't go there, probably it should be an analysis hanging off the side like AssumptionCache is.

================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:499
@@ -495,3 +498,3 @@
       // others.  GV may even be a declaration, not a definition.
-      if (!GV->isConstant()) {
+      if (!GV->isConstant() && !InvInfo.GetStartInstruction(GV)) {
         Visited.clear();
----------------
This seems like a really bad interface. The Pass which sets up the InvInfo has to decide whether to add or not add an invariant intrinsic based on which instruction the caller of pointsToConstantMemory is going to be reasoning about. pointsToConstantMemory could be called by a utility used by the pass, and referring to a point in the program before (or after) the call to invariant.start/end.

================
Comment at: lib/IR/InvariantInfo.cpp:1
@@ +1,2 @@
+//===---------- InvariantInfo.cpp - invariant_start/end info ----*- C++ -*-===//
+//
----------------
Remove the "-*- C++ -*-" part, that's only for .h files.


http://reviews.llvm.org/D14003