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

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy added inline comments.


================
Comment at: docs/LangRef.rst:15085
+it is guaranteed that any returned value leads to correct
+program execution and creates no undefined behavior in code.
+
----------------
I think this spec should just be "The intrinsic ``@llvm.experimental.widenable.condition()`` always non-deterministically returns `true` or `false`."  The ", and it is guaranteed that any returned value leads to correct program execution and creates no undefined behavior in code" bit is true of everything in LLVM IR -- the frontend has to ensure the IR it generated is correct and doesn't have UB.

I'd also emphasize that every invocation of this intrinsic produces a single well defined value non-deterministically (so it isn't like `undef`).


================
Comment at: docs/LangRef.rst:15146
+condition of a guard represented as explicit branch, it is
+always legal to widen the guard's condition with any additional
+conditions.
----------------
Drop the "always".


================
Comment at: docs/LangRef.rst:15161
+
+  %widenable_cond = call i1 @llvm.experimental.widenable.condition()
+  %new_cond = and i1 %any_other_cond, %widenable_cond
----------------
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?


================
Comment at: docs/LangRef.rst:15174
+
+It is always correct to replace the result of
+``@llvm.experimental.widenable.condition``
----------------
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.


https://reviews.llvm.org/D51207