[PATCH] D75726: [ConstExprPreter] Updated constant interpreter documentation

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith marked an inline comment as done.
rsmith added a comment.
This revision is now accepted and ready to land.

Thanks! I'm happy for you to go ahead landing patches that implement the direction documented here. (Let me know if you want me to review them anyway, otherwise I'm happy to assume you've had someone else look over them already.)



================
Comment at: clang/docs/ConstantInterpreter.rst:223-224
+ * **TargetPointer**: represents a target address derived from a base address 
+ through pointer arithmetic, such as ``((int *)0x100)[20]``. Null pointers are 
+ target pointers with a zero offset.
+ * **TypeInfoPointer**: tracks information for the opaque type returned by 
----------------
nand wrote:
> rsmith wrote:
> > This seems problematic -- we need to distinguish between pointers with value zero and null pointers to support targets where they are different. Perhaps we could instead represent null pointers as a new tag in the tagged union, and reserve a zero `TargetPointer` for only those cases where a pointer is zero but not null? (See `APValue::isNullPointer()` and `LValue::IsNullPtr` for how we track that in the old constant evaluator.)
> Hmm, I was not aware of this - another item could be added to the union without too much problem. Do you have an example of such a target?
It looks like the only case where this happens right now is for the OpenCL local address space on AMDGPU. Example:
```
echo 'unsigned long f() { constexpr local int *p = 0; constexpr unsigned long n = __builtin_constant_p(1) ? (unsigned long)p : 0; return n; }' | ./build/bin/clang -x cl -cl-std=clc++ -target amdgcn - -emit-llvm -S -o -
```
reveals the null pointer value there is -1.


================
Comment at: clang/docs/ConstantInterpreter.rst:328
+banned in constexpr, some expressions on target offsets must be folded, 
+replicating the behavious of the ``offsetof`` builtin. Target pointers 
+are characterised by 3 offsets: a field offset, an array offset and a 
----------------
rsmith wrote:
> behavious -> behaviour
(Typo still present.)


================
Comment at: clang/docs/ConstantInterpreter.rst:329-334
+are characterised by 3 offsets: a field offset, an array offset and a 
+base offset, along with a descriptor specifying the type the pointer is 
+supposed to refer to. Array indexing ajusts the array offset, while the 
+field offset is adjusted when a pointer to a member is created. Casting 
+an integer to a pointer sets the value of the base offset. As a special 
+case, null pointers are target pointers with all offets set to 0.
----------------
nand wrote:
> rsmith wrote:
> > Why do we need all three of these? If this is replacing the `APValue` forms with a null base and only an offset, a single value seems sufficient.
> Required for compatibility with the atIndex() method, which is used by the opcode computing array offsets.
OK. Tracking all three seems like it shouldn't be necessary for correctness, but if it makes the implementation easier, then that seems fine.


================
Comment at: clang/docs/ConstantInterpreter.rst:331
+base offset, along with a descriptor specifying the type the pointer is 
+supposed to refer to. Array indexing ajusts the array offset, while the 
+field offset is adjusted when a pointer to a member is created. Casting 
----------------
rsmith wrote:
> ajusts -> adjusts
(Typo still present.)


================
Comment at: clang/docs/ConstantInterpreter.rst:166
 
-Descriptor are generated at bytecode compilation time and contain information required to determine if a particular memory access is allowed in constexpr. Even though there is a single descriptor object, it encodes information for several kinds of objects:
+The interpreter distinguished 3 different kinds of blocks:
 
----------------
distinguished -> distinguishes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75726