[PATCH] D142388: [clang] Add builtin_nondet

Tue Jan 24 06:56:32 PST 2023

ManuelJBrito added inline comments.

================
Comment at: clang/include/clang/Basic/Builtins.def:658
 BUILTIN(__builtin_call_with_static_chain, "v.", "nt")
+BUILTIN(__builtin_nondet, "v.", "nt")

----------------
erichkeane wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > shafik wrote:
> > > > erichkeane wrote:
> > > > > 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 :) 
> > > > `__builtin_indeterminate_value` wdyt?
> > > 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.
> > Why does this require custom typechecking?
> 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 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`? 

================
Comment at: clang/include/clang/Basic/Builtins.def:658
 BUILTIN(__builtin_call_with_static_chain, "v.", "nt")
+BUILTIN(__builtin_nondet, "v.", "nt")

----------------
ManuelJBrito wrote:
> erichkeane wrote:
> > aaron.ballman wrote:
> > > aaron.ballman wrote:
> > > > shafik wrote:
> > > > > erichkeane wrote:
> > > > > > 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 :) 
> > > > > `__builtin_indeterminate_value` wdyt?
> > > > 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.
> > > Why does this require custom typechecking?
> > 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 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`? 
> Why does this require custom typechecking?

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

================
Comment at: clang/include/clang/Basic/Builtins.def:658
 BUILTIN(__builtin_call_with_static_chain, "v.", "nt")
+BUILTIN(__builtin_nondet, "v.", "nt")

----------------
ManuelJBrito wrote:
> ManuelJBrito wrote:
> > erichkeane wrote:
> > > aaron.ballman wrote:
> > > > aaron.ballman wrote:
> > > > > shafik wrote:
> > > > > > erichkeane wrote:
> > > > > > > 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 :) 
> > > > > > `__builtin_indeterminate_value` wdyt?
> > > > > 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.
> > > > Why does this require custom typechecking?
> > > 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 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`? 
> > Why does this require custom typechecking?
> 
> I wasn't sure how to have the builtin take one argument of any type. 
> 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?

================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:3064
+  case Builtin::BI__builtin_nondet: {
+    Value *Result = PoisonValue::get(ConvertType(E->getArg(0)->getType()));
+    Result = Builder.CreateFreeze(Result);
----------------
erichkeane wrote:
> 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
> 
> 
The lowering seems to be as intended. I will add the tests.

================
Comment at: clang/test/CodeGen/builtins-nondet.c:3
+
+typedef float float4 __attribute__((ext_vector_type(4)));
+
----------------
shafik wrote:
> erichkeane wrote:
> > Add some examples for 'bool' and vector-bool as well to make sure this is doing what you want.
> Does this also work for structs?
> Does this also work for structs?

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

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142388/new/

https://reviews.llvm.org/D142388