[PATCH] D136737: [Draft] [clang] Add builtin_unspecified_value

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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



================
Comment at: clang/include/clang-c/Index.h:1534-1539
+  /**
+   * A clang-builtin of unspecified value.
+   */
+  CXCursor_UnspecifiedValueExpr = 155,
+
+  CXCursor_LastExpr = CXCursor_UnspecifiedValueExpr,
----------------
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.


================
Comment at: clang/include/clang/AST/Expr.h:4637-4639
+/// UnspecifiedValueExpr - clang-specific value __builtin_unspecified_value.
+/// This AST node represents freeze(poison) in LLVM IR.
+class UnspecifiedValueExpr : public Expr {
----------------
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.)


================
Comment at: clang/include/clang/AST/Expr.h:4647
+        TokenLoc(Loc) {
+    setDependence(ExprDependence::None);
+  }
----------------
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?


================
Comment at: clang/lib/Sema/TreeTransform.h:11725
+TreeTransform<Derived>::TransformUnspecifiedValueExpr(UnspecifiedValueExpr *E) {
+  return E;
+}
----------------
@erichkeane -- is this correct, or does work need to be done to instantiate the underlying type if the expression is dependent?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136737