[PATCH] D51207: Introduce llvm.experimental.widenable_condition intrinsic

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.

Overall, looks pretty good.  One rounds of comments, but I'm expecting this to converge quickly.

I think there's a missing piece here.  How do widennable_conditions themselves get lowered?  I think that belongs to be in this patch so that it's a whole contained, end to end piece.

A reasonable choice would be to say that CGP or SelectionDAG just lowers them directly to the constant false.



================
Comment at: docs/LangRef.rst:15109
+
+  %widenable_cond = call i1 @llvm.experimental.widenable.condition()
+  and i1 %widenable_cond, %any_other_cond
----------------
The text here needs tweaked.  I'd suggest:
%widenable_cond_orig = call i1 @llvm.experimental.widenable.condition()
%widenable_cond =  and i1 %widenable_cond_orig, %any_other_cond


================
Comment at: docs/LangRef.rst:15119
+``@llvm.experimental.widenable.condition``
+with either `true` or `false`, assuming that the value `true`
+will lead execution to `guarded` block (if all other conditions
----------------
You're wording here is problematic.  As written, it's only legal to lower *if* the else leads to a deopt block which I'm pretty sure is not what you meant.  I think you can just drop everything after "either true or false."


================
Comment at: docs/LangRef.rst:15125
+
+``@llvm.experimental.widenable.condition`` cannot be invoked.
+
----------------
This doesn't sound like part of lowering.  

Why don't you move this to semantics and reword it as:
"wcond" will never throw an exception and thus cannot be invoked.


================
Comment at: include/llvm/Transforms/Scalar/ExplicifyGuards.h:1
+//===--- ExplicifyGuards.h - Turn guard intrinsics into guard branches ----===//
+//
----------------
I'm not actively objecting to the structure here, but this really just feels like another form of guard lowering.  Maybe if would be better to reuse the existing implementation and just have a parameter to control which form we're lowing to?  LowerGuards vs LowerGuardsToWidennableConds?


================
Comment at: lib/Transforms/Scalar/ExplicifyGuards.cpp:80
+
+static bool explicifyGuards(Function &F) {
+  // Check if we can cheaply rule out the possibility of not having any work to
----------------
"explicify" as a verb feels very awkward.  "MakeGuardsExplicit" would be more clear.  


https://reviews.llvm.org/D51207