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.
----------------
[email protected] wrote:
> [email protected] wrote:
> > [email protected] 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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
