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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc 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.
+
----------------
sanjoy wrote:
> 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`).
I don't think "always" adds much value here.

I also think "non-deterministically" can be confusing to the reader as this doesn't cause the *compiler* to fold them non-deterministically.

I wonder if we could phrase the semantics more like:

```
The intrinsic ``...`` returns either `true` or `false`. For each evaluation of a call to this intrinsic, the program must be valid and correct both if it returns `true` and if it returns `false`. This allows transformation passes to replace evaluations of this intrinsic with either value whenever one is beneficial.
```

Uncertain how others feel about this approach to the semantics.


================
Comment at: docs/LangRef.rst:15174
+
+It is always correct to replace the result of
+``@llvm.experimental.widenable.condition``
----------------
mkazantsev wrote:
> 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.
I don't think we need to allow passing in the directionality... FEs can choose to emit the alternatives in the structure necessary?

But I do like the point that maybe the fact that one is the "default" should be evident to the FE author so that they can choose the structure correctly.


================
Comment at: docs/LangRef.rst:15134-15137
+This intrinsic represents a "widenable condition" which is
+boolean expressions with the following property: whether this
+expression is `true` or `false`, the program is correct and
+well-defined.
----------------
I really like this proposal, but the term "widenable condition" really doesn't help me understand it at all.

Have you all thought of other terminology that might work here?

Some ideas:

"equivalent condition"
"equivalent alternative condition"

Something focusing on the fact that this intrinsic models a value which can inhabit two alternative states that are semantically equivalent when executed.

Thoughts?


================
Comment at: docs/LangRef.rst:15145-15148
+It is somewhat close to `undef` in definition, but is deliberately
+used to perform guard widening and similar transforms and does not
+have such properties of `undef` as ability to change value during the
+live range.
----------------
I would phrase this the other way around. I think the important thing to mention about `undef` here is its *difference*. I also wouldn't focus first on the *use case*, but the *semantic* difference:

```
While this may appear similar in semantics to `undef`, it is very different in that an invocation produces a particular, singular value. It is also intended to be lowered late, and remain available for specific optimizations and transforms that can benefit from its special properties.
```


https://reviews.llvm.org/D51207