[clang] Add builtin_nondeterministic_value

[ManuelJBrito]

Summary

This patch adds a builtin that returns a non deterministic value of the same type as the argument.

Example:
int x = __builtin_nondeterministic_value(x);

Which is then lowered to freeze(poison).

This is a continuation of the work done in https://reviews.llvm.org/D136737, following the provided feedback.

[erichkeane]

clang/include/clang/Basic/Builtins.def

658

Not a fan of the name in general, it doesn't really explain what is happening. Perhaps __builtin_nondeterministic_value or something?

I also wonder if others might have a better name :)

clang/lib/CodeGen/CGBuiltin.cpp

3064

Is this troublesome with some of the types that have a ConvertTypeForMem? That is, bool and bool vector types might need a different type/representation? https://clang.llvm.org/doxygen/CodeGenTypes_8cpp_source.html#l00091

clang/test/CodeGen/builtins-nondet.c

3

Add some examples for 'bool' and vector-bool as well to make sure this is doing what you want.

[shafik]

clang/include/clang/Basic/Builtins.def

658

__builtin_indeterminate_value wdyt?

clang/test/CodeGen/builtins-nondet.c

3

Does this also work for structs?

[aaron.ballman]

Be sure to add documentation for this to the language extensions page and mention the new builtin in the release notes.

clang/include/clang/Basic/Builtins.def

658

I think __builtin_unspecified_value() makes sense, as that's what you're getting back (there is a valid value, but you may get a different value on every call). I don't think indeterminate makes sense because that implies it may return trap values, and I'm guessing that's not behavior we want here (or am I wrong about that)?

Also, should this be usable within a constant expression? I would assume not; we should have a test case showing you can't use it in such a context.

658

Why does this require custom typechecking?

[erichkeane]

clang/include/clang/Basic/Builtins.def

658

Our issue last time with "unspecified" value was that this has a distinct meaning in C/C++, so we don't want to use that word.

[ManuelJBrito]

clang/include/clang/Basic/Builtins.def

658

I don't think indeterminate makes sense because that implies it may return trap values, and I'm guessing that's not behavior we want here (or am I wrong about that)?

The idea is for it to return some correct value of the same type as the argument, so no trap values.

Can we settle on __builtin_nondeterministic_value?

658

Why does this require custom typechecking?

I wasn't sure how to have the builtin take one argument of any type.

658

Also, should this be usable within a constant expression? I would assume not; we should have a test case showing you can't use it in such a context.

Can you shed a light on the implications of this? Or point me to some reference?

clang/lib/CodeGen/CGBuiltin.cpp

3064

The lowering seems to be as intended. I will add the tests.

clang/test/CodeGen/builtins-nondet.c

3

Does this also work for structs?

No, i don't think so. What would be expected IR?

[erichkeane]

clang/test/CodeGen/builtins-nondet.c

3

can freeze/posion not be of struct types? If they don't work, it should be an error to try.

[ManuelJBrito]

Add tests with bool and bool-vector

[ManuelJBrito]

Oops wrong diff, i'll update it.

[ManuelJBrito]

fixed diff

[aaron.ballman]

clang/include/clang/Basic/Builtins.def

658

Can we settle on __builtin_nondeterministic_value?

I can live with it.

Our issue last time with "unspecified" value was that this has a distinct meaning in C/C++, so we don't want to use that word.

I guess I don't see the issue; the behavior of the builtin as I understand it matches that distinct meaning in C/C++ (and English prose).

Also, should this be usable within a constant expression? I would assume not; we should have a test case showing you can't use it in such a context.

Can you shed a light on the implications of this? Or point me to some reference?

It boils down to whether you think this should be valid:

constexpr bool hoo_boy() {
  return __builtin_nondeterministic_value() == 42;
}
constexpr bool val = hoo_boy();

(among other uses of constant expressions). My intuition is that there's nothing useful you could do with this builtin in a constant expression (my example is generally always going to be false except for the times when it's true, lol), but if all we're doing is giving you *a* value, nothing says we can't generate that value at compile time so it seems possible to support.

Why does this require custom typechecking?

I wasn't sure how to have the builtin take one argument of any type.

I was thinking it can use . to specify that the function is varargs, and then SemaChecking.cpp can validate that only one argument is passed to the call, but now that I think about that.... maybe we don't want that behavior because that would induce default argument promotions which would change the type (for example, from float to double). So on reflection, I think this is fine as-is.

[ManuelJBrito]

• Change name to __builtin_nondeterministic_value
• Add struct support

[erichkeane]

clang/lib/CodeGen/CGBuiltin.cpp

3070

This gets REALLY complicated, you can't just create a store, this might end up hitting conversion operators/etc, and is subject to triviality/etc, and also probably needs to go through a constructor. I suspect you're going to prefer to just decide this isn't a valid builtin for structs instead of getting bogged down in that mess.

clang/lib/Sema/SemaChecking.cpp

17814

So I think choosing the sub-set of types here is a much better idea, allowing any of the user-defined types is going to make your codegen for this builtin a mess. IMO, just isBuiltinType + isVectorType is probably an acceptable list for now.

clang/test/CodeGen/builtins-nondeterministic-value.c

6 ↗

(On Diff #493904)

If you're going to allow this, we need tests for unions, constructors, non-trivial types, copy assignment/etc.

[ManuelJBrito]

clang/lib/CodeGen/CGBuiltin.cpp

3070

The motivation for this builtin was to match intel's definition of undefined in the lowering of intrinsics such as https://www.intel.com/content/www/us/en/docs/intrinsics-guide/index.html#text=_mm256_castsi128_si256&expand=628&ig_expand=755, so struct support isn't critical.
So if everyone else agrees i'll drop struct support, do as you suggest and if there are use cases for nondeterministic values of other types add support for them later?

[erichkeane]

clang/lib/CodeGen/CGBuiltin.cpp

3070

I suspect I speak for all with my suggestion. I don't think Aaron/Shafik are concerned with struct support here.

[aaron.ballman]

clang/lib/CodeGen/CGBuiltin.cpp

3070

If we reject the code, we can relax that restriction in the future without breaking code, so I think it's fine to not support structures. We can add support for them later if we have a need to do so.

[xbolva00]

Please add a release note.

Thanks for this new builtin, as I was working on something very very similar - the implementation looks fine to me.

[ManuelJBrito]

• Restrict builtin to base types and vectors

[ManuelJBrito]

Can the release note and documentation update be part of this patch or should i create a new one ?

[erichkeane]

In D142388#4097136, @ManuelJBrito wrote:

Can the release note and documentation update be part of this patch or should i create a new one ?

They should be a part of this patch.

[erichkeane]

clang/lib/Sema/SemaChecking.cpp

17810

This should set the expression as invalid, shouldn't it? That way we don't end up depending on it too much.

[ManuelJBrito]

• set Invalid in type checking
• add builtin documentation
• add release note

[erichkeane]

Last set of comments, docs and release notes look fine to me. feel free to fix the last comments while committing.

clang/lib/Sema/SemaChecking.cpp

17810

17812

Diag always returns true for exactly this situation, omit the {} and return true;, and just return Diag.

[ManuelJBrito]

Thank you all for the reviews and helping me see this patch through.

[ManuelJBrito]

So this is getting some build failures:

• https://lab.llvm.org/buildbot#builders/38/builds/9446
• https://lab.llvm.org/buildbot#builders/245/builds/4189
• https://lab.llvm.org/buildbot#builders/65/builds/7949
• https://lab.llvm.org/buildbot#builders/188/builds/25538

I suspect it's because of the way different ABIs handle the vectors. I was thinking that if we have the variable be a local variable and also not returning the value can fix the tests.

Does this make sense?

[thakis]

Looks like the test fails on win: http://45.33.8.238/win/74290/step_7.txt

And Mac: http://45.33.8.238/macm1/54080/step_7.txt

Please take a look and revert for now if it takes a while to fix.

[thakis]

Reverted in adf7ffd51ee34c3a72d3168f5aed8b946ba3d2cc for now.

[erichkeane]

In D142388#4099374, @ManuelJBrito wrote:

So this is getting some build failures:

• https://lab.llvm.org/buildbot#builders/38/builds/9446
• https://lab.llvm.org/buildbot#builders/245/builds/4189
• https://lab.llvm.org/buildbot#builders/65/builds/7949
• https://lab.llvm.org/buildbot#builders/188/builds/25538

I suspect it's because of the way different ABIs handle the vectors. I was thinking that if we have the variable be a local variable and also not returning the value can fix the tests.

Does this make sense?

Yep, thats what it looks like, ABI differences. Feel free to update this review with the new test now that you've been reverted, and I'll look to see if it seems sufficient.

[ManuelJBrito]

Fix vector tests

[erichkeane]

Other than the newline, the test change is acceptable to me. Commit when ready :)

clang/test/CodeGen/builtins-nondeterministic-value.c

61 ↗

(On Diff #494338)

We do need a newline here.