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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev marked 2 inline comments as done.
mkazantsev added inline comments.


================
Comment at: docs/LangRef.rst:15161
+
+  %widenable_cond = call i1 @llvm.experimental.widenable.condition()
+  %new_cond = and i1 %any_other_cond, %widenable_cond
----------------
sanjoy wrote:
> Not sure if this belongs in the langref, but the intrinsic must be RAUW'ed with the stronger condition, replacing just one use is unsound right?
I see no problem in replacing only one use. It maybe makes no sense, but by definition it should be no bug.


================
Comment at: docs/LangRef.rst:15174
+
+It is always correct to replace the result of
+``@llvm.experimental.widenable.condition``
----------------
sanjoy wrote:
> This is an important detail; not from a semantics perspective but from a performance perspective.  I'm wondering if this behavior should be a part of name of the intrinsic (or maybe even that the intrinsic should have an argument which is what we default lower the intrinsic to).
> 
> For instance, given this spec it would be correct but unwise to lower a range check to:
> 
> ```
> %w = widenable_cond();
> if (%w || out_of_bounds()) deoptimize();
> ```
> 
> but there is nothing in its name that makes this obvious.
I think that the last sentence about the default lowering strategy implies this, but actually it is OK to implement a different lowering strategy if at some use case this one is non-profitable.


https://reviews.llvm.org/D51207