[PATCH] Invariants (and Assume Aligned) - Clang

[hfinkel at anl.gov]

Thanks! Will post updated patch soon.

================
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.
----------------
hfinkel at anl.gov wrote:
> hfinkel at anl.gov wrote:
> > hfinkel at anl.gov wrote:
> > > Richard Smith wrote:
> > > > 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.
> > > Interesting point. MS's documentation says, "no code is generated", but does not otherwise specify the semantics of what is done with the arguments. With the LLVM intrinsic, although IR is generated, no assembly is generated.
> > > 
> > > Are you asking whether __assume is more like decltype? Or could __assume(*a > 0) trap if a were nullptr? I'm pretty sure that the answers are no and yes, respectively. MS's documentation specifically talks about using __assume as a production-build replacement for assert, and I assume so must evaluate its inputs in a similar-enough way to make that workable.
> > After writing that, it occurred to me that whether or not assert evaluates its arguments depends on the preprocessor state, so there is little to infer from that comparison.
> > 
> > What is true is that MS has a compiler warning: C4557 (The value passed to an __assume statement was modified.) which triggers on code like __assume(i++).
> > 
> echristo just confirmed for me on IRC that __assume does not evaluate its arguments. I'll need to filter for side effects.
Done.

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

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

================
Comment at: lib/CodeGen/CGBuiltin.cpp:418
@@ +417,3 @@
+  }
+  case Builtin::BI__assume:
+  case Builtin::BI__builtin_assume: {
----------------
Richard Smith wrote:
> Is this really correct? As noted above, I think `__assume` doesn't evaluate its argument. Perhaps I'm mistaken.
No, you're right. I added a side-effects check.

================
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"
----------------
Richard Smith wrote:
> I'm not happy about this dependency. But see below...
Removed.

================
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);
----------------
Richard Smith wrote:
> This is a bit confusing. These seem to be two entirely unrelated facts (the alignment is argument 1).
Agreed. I removed the bit about argument 0 (I'm not sure it really adds anything).

================
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())
----------------
Richard Smith wrote:
> 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.
Good point.

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

================
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))
----------------
Richard Smith wrote:
> I don't see how LLVM's supported alignment values have any impact here?
Fair enough.

================
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();
----------------
Richard Smith wrote:
> 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;
Thanks!

http://reviews.llvm.org/D149