[PATCH] D18527: Introduce a @llvm.experimental.guard.on intrinsic

[Sanjoy Das via llvm-commits]

sanjoy added inline comments.

================
Comment at: docs/LangRef.rst:12202
@@ +12201,3 @@
+	define void @llvm.experimental.guard.on(i1 %pred, <args>) {
+	  %realPred = and i1 %pred, undef
+	  br i1 %realPred, label %continue, label %leave
----------------
reames wrote:
> minor: instead of undef, maybe a volatile load from an unmodeled location?
I think that'd be the opposite of what we want.  "volatile load from an unmodeled location" => the optimizer needs to model it most conservatively (i.e. can be any value the programmer wants), "undef" => the optimizer can model it least conservatively (i.e. can be any value the optimizer wants).

================
Comment at: lib/Transforms/Scalar/LowerGuardIntrinsic.cpp:15
@@ +14,3 @@
+
+#include "llvm/Transforms/Scalar.h"
+#include "llvm/ADT/SmallVector.h"
----------------
reames wrote:
> Include sort.
The convention in the other files (that include `Scalar.h`) seem to be to include `Scalar.h` and then have the rest of the list sorted.

================
Comment at: lib/Transforms/Scalar/LowerGuardIntrinsic.cpp:39
@@ +38,3 @@
+bool LowerGuardIntrinsic::runOnFunction(Function &F) {
+  SmallVector<CallInst *, 8> ToLower;
+
----------------
reames wrote:
> Possibly add a fastpath of the form:
> if (getFunction(guard)->uses_empty())
>   return;
> 
> I'd like this to be on by default in the standard pipeline.
Done, but that's not enough to switch this on in the standard pipeline.  As written, in the old pass manager, this will still break all analyses in the no-op case.  We can consider fixing that, but not in this change.

================
Comment at: lib/Transforms/Scalar/LowerGuardIntrinsic.cpp:54
@@ +53,3 @@
+  for (auto *CI : ToLower) {
+    // Semantically this is just inlining, but inlining around varargs is
+    // complicated.
----------------
reames wrote:
> I find this comment more confusing then helpful.
Removed.

================
Comment at: test/Transforms/LowerGuardIntrinsic/basic.ll:1
@@ +1,2 @@
+; RUN: opt -S -lower-guard-intrinsic < %s | FileCheck %s
+
----------------
reames wrote:
> Hm, it looks like the lowering pass is not part of the standard pipeline right now.  I'd like to see this become part of the standard LLC pipeline.  Doesn't have to be in this change specifically, but I will expect to see lowering tests showing the a guard gets lowered correctly all the way to assembly at some point.
As mentioned earlier, adding this pass as is to the default pipeline is probably going to be a compile time regression.

================
Comment at: test/Transforms/LowerGuardIntrinsic/basic.ll:5
@@ +4,3 @@
+
+define i8 @f0(i1* %c_ptr) alwaysinline {
+; CHECK-LABEL: @f0(
----------------
reames wrote:
> Why alwaysinline?
Leftover from where I copied the test from, removed.

================
Comment at: test/Transforms/LowerGuardIntrinsic/basic.ll:19
@@ +18,3 @@
+
+define void @f1(i1* %c_ptr) alwaysinline {
+; CHECK-LABEL: @f1(
----------------
reames wrote:
> Can you comment what each is testing?
I didn't add comments, but renamed the functions to add some context.


http://reviews.llvm.org/D18527