[PATCH] Invariants (and Assume Aligned) - Clang

[Richard Smith]

================
Comment at: docs/LanguageExtensions.rst:1267-1269
@@ +1266,5 @@
+
+The boolean argument to this function is defined to be true. The optimizer may
+analyze the expression used to compute the argument and deduce from that
+information used to optimize the program. If the condition is violated during
+execution, the behavior is undefined.
----------------
You should indicate whether and when the condition is evaluated here. IIRC, MS' `__assume` does not evaluate its argument, so if this builtin differs from that we should be explicit about that difference.

================
Comment at: lib/AST/ExprConstant.cpp:6089
@@ -6088,2 +6088,3 @@
 
+  case Builtin::BI__builtin_assume_aligned:
   case Builtin::BI__builtin_expect:
----------------
Please also add handling of `BI__builtin_assume` to `VoidExprEvaluator`.

================
Comment at: lib/CodeGen/CGBuiltin.cpp:407
@@ +406,3 @@
+                        /*isSigned*/true, "offsetcast");
+      PtrIntValue = Builder.CreateSub(PtrIntValue, OffsetValue, "offsetptr");
+    }
----------------
Skip this if the `OffsetValue` is the `ConstantInt` 0? (I'm kinda surprised the `IRBuilder` doesn't already do that...)

================
Comment at: lib/CodeGen/CGBuiltin.cpp:418
@@ +417,3 @@
+  }
+  case Builtin::BI__assume:
+  case Builtin::BI__builtin_assume: {
----------------
Is this really correct? As noted above, I think `__assume` doesn't evaluate its argument. Perhaps I'm mistaken.

================
Comment at: lib/Sema/SemaChecking.cpp:38
@@ -37,2 +37,3 @@
 #include "llvm/ADT/SmallString.h"
+#include "llvm/IR/Value.h"
 #include "llvm/Support/ConvertUTF.h"
----------------
I'm not happy about this dependency. But see below...

================
Comment at: lib/Sema/SemaChecking.cpp:1961
@@ +1960,3 @@
+
+  // Argument 0 is checked for us; the alignment must be a constant integer.
+  Expr *Arg = TheCall->getArg(1);
----------------
This is a bit confusing. These seem to be two entirely unrelated facts (the alignment is argument 1).

================
Comment at: lib/Sema/SemaChecking.cpp:1970-1973
@@ +1969,6 @@
+
+    if (Result.getLimitedValue() > +llvm::Value::MaximumAlignment ||
+        !Result.isUnsigned())
+      return Diag(TheCall->getLocStart(), diag::err_argument_invalid_range)
+           << "0" << +llvm::Value::MaximumAlignment << Arg->getSourceRange();
+    if (!Result.isPowerOf2())
----------------
What's the purpose of this check? If I want to claim that my pointer is `0x8000'0000'0000ull` bytes aligned, I don't see why we should diagnose that, even if LLVM can't represent that alignment. CodeGen doesn't care about this being a valid LLVM alignment.

================
Comment at: lib/Sema/SemaChecking.cpp:1976
@@ +1975,3 @@
+      return Diag(TheCall->getLocStart(),
+                  diag::err_attribute_aligned_not_power_of_two)
+           << Arg->getSourceRange();
----------------
This seems like an odd diagnostic ID to reuse. If you want to use it, please change its name to not mention an attribute.

================
Comment at: lib/Sema/SemaChecking.cpp:1984-1985
@@ +1983,4 @@
+
+    // Note: gcc specifies the prototype as taking a size_t, but LLVM can
+    // handle only 32-bit alignment values.
+    if (!T->isIntegralType(Context))
----------------
I don't see how LLVM's supported alignment values have any impact here?

================
Comment at: lib/Sema/SemaChecking.cpp:1988
@@ +1987,3 @@
+      return Diag(TheCall->getLocStart(),
+               diag::err_typecheck_converted_constant_expression)
+          << T << "an integral type" << Arg->getSourceRange();
----------------
Again, this is a weird diagnostic ID to reuse. This has nothing to do with converted constant expressions. (And you never even tried to do any conversion, so you can't reasonably claim the argument is not implicitly convertible to a particular type.)

Instead of all this, please perform the appropriate conversion from the argument to type `size_t`:

    InitializedEntity Entity = InitializedEntity::InitializeParameter(Context, Context.getSizeType(), false);
    Arg = PerformCopyInitialization(Entity, SourceLocation(), Arg);
    if (Arg.isInvalid()) return true;

http://reviews.llvm.org/D149