[PATCH] D15124: Use @llvm.invariant.start/end intrinsics to extend basic AA with invariant range analysis for GVN-based load elimination purposes [Local objects only]

[Hal Finkel via llvm-commits]

hfinkel added a comment.

> Benchmarking put aside, could someone please remind what the fundamental concern to pushing this in right now is?


It's been a while, so I apologize if I'm off base on this...

I recall being concerned about the use of single-invariant-end postdominance here, because it is not, fundamentally, the correct property. You don't care that a single invariant-end call postdominates the access, but that the set of all such calls do. As you allude to in the PDT change, once exceptions are enabled, there are multiple paths on which a destructor is called. If I assume that, in general, you want an invariant-start intrinsic after the constructor call, and an invariant-end call before the destructor call, then you need to deal with this property. Even if Clang always generates one cleanup block, and even when exceptions are disabled, nothing prevents other passes from "tail merging" the cleanup block. Given that, with exceptions enabled, we already have the more-general case of multiple ends per start, we should design this around a scheme that can handle that situation.


================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:797
@@ +796,3 @@
+  // NOTE: Only call instructions for now.
+  // FIXME: Allow all call sites.
+  if (const CallInst *CI = dyn_cast<CallInst>(CS.getInstruction())) {
----------------
By "all call sites" what do you mean? Invokes? Please either make the FIXME more explanatory, or remove it.

================
Comment at: lib/Analysis/PostDominators.cpp:53-68
@@ +52,18 @@
+  // post-dominates the instruction.
+  // TODO: Special case for invoke instructions:
+  //       Allow invoke instructions to be post-dominated by instructions in
+  //       their normal destination path to allow further optimizations from
+  //       invariant range analysis such as the elimination of the second load
+  //       instruction in
+  //
+  //          <code>
+  //          try { ...
+  //                @llvm.invariant.start(..., bitcast i32* %i to i8*)
+  //                load %i
+  //                invoke(...)
+  //                load %i
+  //                ...
+  //                @llvm.invariant.end(..., bitcast i32* %i to i8*)
+  //          } catch (...) { ... }
+  //          </code>
+  //
----------------
I don't understand your response. Modifying the definition of post-dominance to add a special case for exception unwinding desired by the invariant analysis seems unwise - i.e. likely to lead to bugs in other users of this code.

================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:1982
@@ +1981,3 @@
+      } else if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(I)) {
+        if (II->getIntrinsicID() == Intrinsic::objectsize) {
+          ConstantInt *CI = cast<ConstantInt>(II->getArgOperand(1));
----------------
Why is Intrinsic::objectsize handling in this patch?


http://reviews.llvm.org/D15124