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

[Philip Reames via llvm-commits]

reames added a comment.

A bunch of small comments.  Looks close to ready to go in.

I think it would also help to layout a quick sketch of what comes next.  How does this fit into the standard pipeline?  What optimization passes are going to be changed?  How does this interact with assume?


================
Comment at: docs/LangRef.rst:12179
@@ -12178,1 +12178,3 @@
 
+'``llvm.experimental.guard.on``' Intrinsic
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
----------------
Minor: I'd drop on from the name.  The extra punctuation is distracting.

================
Comment at: docs/LangRef.rst:12201
@@ +12200,3 @@
+
+	define void @llvm.experimental.guard.on(i1 %pred, <args>) {
+	  %realPred = and i1 %pred, undef
----------------
<args...>

Make a note about the var args part..

================
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
----------------
minor: instead of undef, maybe a volatile load from an unmodeled location?

================
Comment at: docs/LangRef.rst:12213
@@ +12212,3 @@
+
+In words, ``@llvm.experimental.guard.on`` deoptimizes the current
+physical frame if (but **not** only if) its first argument is
----------------
wording: I wouldn't use deoptimize in this sense.  It could be confused with policy choices like what to do with the code.  

================
Comment at: docs/LangRef.rst:12215
@@ +12214,3 @@
+physical frame if (but **not** only if) its first argument is
+``false``.
+
----------------
Mention widening explicitly as motivation for the guard.

================
Comment at: lib/Transforms/Scalar/LowerGuardIntrinsic.cpp:11
@@ +10,3 @@
+// This pass lowers the llvm.experimental.guard.on intrinsic to a conditional
+// call to @llvm.experimental.deoptimize.
+//
----------------
Clarify that after lowering, the guards can no longer be widened.

================
Comment at: lib/Transforms/Scalar/LowerGuardIntrinsic.cpp:15
@@ +14,3 @@
+
+#include "llvm/Transforms/Scalar.h"
+#include "llvm/ADT/SmallVector.h"
----------------
Include sort.

================
Comment at: lib/Transforms/Scalar/LowerGuardIntrinsic.cpp:39
@@ +38,3 @@
+bool LowerGuardIntrinsic::runOnFunction(Function &F) {
+  SmallVector<CallInst *, 8> ToLower;
+
----------------
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.

================
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.
----------------
I find this comment more confusing then helpful.

================
Comment at: lib/Transforms/Scalar/LowerGuardIntrinsic.cpp:60
@@ +59,3 @@
+
+    auto *CheckBB = CI->getParent();
+    auto *DeoptBlockTerm =
----------------
Pulling out a helper function which inserts the new control flow, calling it, then dropping the original call might be clearer.

================
Comment at: lib/Transforms/Scalar/LowerGuardIntrinsic.cpp:70
@@ +69,3 @@
+
+    CheckBI->getSuccessor(0)->setName("nodeopt");
+    CheckBI->getSuccessor(1)->setName("deopt");
----------------
possibly: "guarded"

================
Comment at: lib/Transforms/Scalar/LowerGuardIntrinsic.cpp:73
@@ +72,3 @@
+
+    auto *DeoptCall =
+        CallInst::Create(DeoptIntrinsic, Args, {DeoptOB}, "", DeoptBlockTerm);
----------------
Two suggestions:
- Use IRBuilder?
- Explicitly branch on whether there's a return type rather than using two ternaries (one explicit, one implicit).  

================
Comment at: test/Transforms/LowerGuardIntrinsic/basic.ll:1
@@ +1,2 @@
+; RUN: opt -S -lower-guard-intrinsic < %s | FileCheck %s
+
----------------
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.

================
Comment at: test/Transforms/LowerGuardIntrinsic/basic.ll:5
@@ +4,3 @@
+
+define i8 @f0(i1* %c_ptr) alwaysinline {
+; CHECK-LABEL: @f0(
----------------
Why alwaysinline?

================
Comment at: test/Transforms/LowerGuardIntrinsic/basic.ll:19
@@ +18,3 @@
+
+define void @f1(i1* %c_ptr) alwaysinline {
+; CHECK-LABEL: @f1(
----------------
Can you comment what each is testing?


http://reviews.llvm.org/D18527