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

[Philip Reames via Phabricator via llvm-commits]

reames accepted this revision.
reames added a comment.
This revision is now accepted and ready to land.

LGTM.

I'm approving this in the current form, despite a bit of hesitation doing so.  I'd like to see the conversation around the restriction of widening based on target block to continue.  I think there's a good change we'll want to tweak the semantics there, but I see that as a minor tweak, not a major redesign.

There are a couple of follow ups I'd like to see here.

1. A default lowering strategy for the new representation.  (Likely in SelectionDAG).
2. (Optional) Extending the existing LowerGuard pass to lower the new form as well.
3. Tests for EarlyCSE, InstCombine, GVN, and DSE which show that two adjacent calls to the widen intrinsic aren't merged and that memory can be forwarded past.  (Note: The first part is profitability, but the second is legality.)



================
Comment at: lib/Transforms/Scalar/MakeGuardsExplicit.cpp:72
+                        ExplicitGuard, "widenable_cond");
+  WidenableCondition->setCallingConv(Guard->getCallingConv());
+  auto *NewCond =
----------------
I don't think this needs to use the guard calling convention.  It'll never get lowered.  The important bit was using this CC on the deopt call which will.  


https://reviews.llvm.org/D51207