[PATCH] D71499: Add builtins for aligning and checking alignment of pointers and integers

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

rsmith added a comment.

To me these seem sufficiently useful to be worth having. Please add documentation for them.



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2913
+def warn_alignment_builtin_useless : Warning<
+  "%select{aligning a value|checking whether a value is aligned}0 to 1 byte is"
+   " %select{a no-op|always true}0">, InGroup<TautologicalCompare>;
----------------
"the result of checking whether [...] is always true" would be better.


================
Comment at: clang/lib/AST/ExprConstant.cpp:10546-10548
+  if (!E->getArg(0)->EvaluateAsInt(ExprResult, Info.Ctx))
+    return false;
+  Val = ExprResult.Val.getInt();
----------------
rsmith wrote:
> These should be evaluated in the same `EvalInfo` as the enclosing evaluation, so they can use (eg) the values of local state that's already been computed in a constexpr function.
There are also cases where the first argument is a pointer for which we should be able to constant-fold `__builtin_is_aligned` (if we can evaluate it to a specific offset within a specific object, and we know the alignment for that object as a whole). See the handling for `__builtin_assume_aligned`, and factor out the common part.


================
Comment at: clang/lib/AST/ExprConstant.cpp:10546-10550
+  if (!E->getArg(0)->EvaluateAsInt(ExprResult, Info.Ctx))
+    return false;
+  Val = ExprResult.Val.getInt();
+  if (!E->getArg(1)->EvaluateAsInt(ExprResult, Info.Ctx))
+    return false;
----------------
These should be evaluated in the same `EvalInfo` as the enclosing evaluation, so they can use (eg) the values of local state that's already been computed in a constexpr function.


================
Comment at: clang/lib/AST/ExprConstant.cpp:10553
+  if (Alignment < 0)
+    return false;
+  // XXX: can this ever happen? Will we end up here even if Sema gives an error?
----------------
You need to produce a diagnostic on failure.


================
Comment at: clang/lib/AST/ExprConstant.cpp:10558
+    return false;
+  // Ensure both values have the same bit width so that we don't assert later.
+  *ValWidth = Val.getBitWidth();
----------------
Would it make more sense to convert integer arguments to `intptr_t` when checking the call to the builtin?


================
Comment at: clang/lib/Sema/SemaChecking.cpp:222
+  QualType SrcTy = Source->getType();
+  // Should also be able to use it with arrays (but not functions!)
+  bool IsArrayToPointerDecay =
----------------
You should actually decay the argument to a pointer here. More generally, the right pattern to use here is:

1) Compute the parameter type you want the builtin to have for this call, then
2) Perform a copy-initialization of a parameter with that type from the argument, then
3) Update TheCall's corresponding argument to the converted form.

You can find this pattern in various builtin checking functions in this file if you want something to crib from. (Search for `InitializeParameter`.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71499