This is an archive of the discontinued LLVM Phabricator instance.

[Draft] [clang] Add builtin_unspecified_value
AbandonedPublic

Authored by aqjune on Oct 25 2022, 10:37 PM.

Details

Reviewers
shafik
NoQ
Summary

This patch adds a builtin constant that lowers to freeze(poison).
This is necessary to patch the intrinsics like e.g., mm256_castsi128_si256 to be lowered to the following sequence:

%a1 = freeze <2 x double> poison // <- would like to represent this as '__builtin_unspecified_value' in C/C++.
%res = shufflevector <2 x double> %a0, <2 x double> %a1, <4 x i32> <i32 0, i32 1, i32 2, i32 3>

Currently it is being lowered to:

%res = shufflevector <2x  double> %a0, undef, <4 x i32> <i32 0, i32 1, i32 -1, i32 -1>

The current lowering may incorrectly introduce undefined behavior.

A related, previous patch was here: https://reviews.llvm.org/D130339

Diff Detail

Event Timeline

aqjune created this revision.Oct 25 2022, 10:37 PM
Herald added a reviewer: NoQ. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
aqjune requested review of this revision.Oct 25 2022, 10:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 25 2022, 10:37 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

The updates are analogous to how GNUNullExprClass is processed because UnspecifiedValueExprClass and it is similar (have no operand).

It doesn't really make sense to me why this is an AST node expr-node-type rather than just being a call to a builtin (that'll likely need to take a var as a parameter)? We can make that builtin result in a freeze/poison value if necessary.

So something like:
_si128 x = __builtin_magic_unspecified(x);

Also, as discussed with @aaron.ballman, 'unspecified' has VERY specific meaning in C/C++, and you don't seem to mean that here.

It looks like this is missing parsing logic and test cases.

clang/include/clang-c/Index.h
1534–1539

I'm not certain we want to expose this via the C APIs. Those APIs are stable APIs and this is exposing an LLVM IR implementation detail (in an area that's traditionally been rather unclear and actively worked on in LLVM IR). I don't know that we're ready to commit to never changing the behavior from LLVM's side.

clang/include/clang/AST/Expr.h
4637–4639

I don't think we should be using unspecified for the terms here -- that has a specific meaning which is not modeled by freeze(poison).

3.19.3
unspecified value
valid value of the relevant type where this document imposes no requirements on which value is chosen in any instance

specifically, freeze causes us to return the same value every time and the interesting property about unspecified values is that each read may give you a different value regardless of whether there's an intervening store (so it's sort of like volatile in that regard).

(FWIW, I think exposing a builtin for generating actually unspecified values [where you get a different value on every execution] would be a more compelling builtin for general use.)

4647

This is incorrect -- you should be calling computeDependence() and implementing the correct logic there. For example, consider calling this builtin on a (partially) dependent type -- we don't want to say the value has no dependence given that we don't know what type it has, right?

clang/lib/Sema/TreeTransform.h
11725

@erichkeane -- is this correct, or does work need to be done to instantiate the underlying type if the expression is dependent?

erichkeane added inline comments.Oct 26 2022, 11:51 AM
clang/lib/Sema/TreeTransform.h
11725

No, a transform on the type does need to happen.

Matt added a subscriber: Matt.Feb 6 2023, 12:46 PM
aqjune abandoned this revision.Feb 22 2023, 8:15 AM

Closing this as D142388 added a function that can be used instead