address comments

Author: Marc Rasi

include/swift/AST/DiagnosticsSIL.def

@@ -303,9 +303,15 @@ ERROR(pound_assert_condition_not_constant,none,
 ERROR(pound_assert_failure,none,
        "%0", (StringRef))
 
-NOTE(constexpr_unknown_reason, none, "%0", (StringRef))
-NOTE(constexpr_called_from, none, "when called from here", ())
-NOTE(constexpr_not_evaluable, none,
+NOTE(constexpr_unknown_reason_default,none, "could not fold operation", ())
+NOTE(constexpr_too_many_instructions,none,
+     "exceeded instruction limit: %0 when evaluating the expression at compile "
+     "time", (unsigned))
+NOTE(constexpr_loop,none, "control flow loop found", ())
+NOTE(constexpr_overflow,none, "integer overflow detected", ())
+NOTE(constexpr_trap,none, "trap detected", ())
+NOTE(constexpr_called_from,none, "when called from here", ())
+NOTE(constexpr_not_evaluable,none,
      "expression not evaluable as constant here", ())
 
 ERROR(non_physical_addressof,none,

include/swift/SIL/SILConstants.h

@@ -121,15 +121,15 @@ class SymbolicValue {
 
   union {
     /// This is the reason code for RK_Unknown values.
-    UnknownReason unknown_reason : 32;
+    UnknownReason unknownReason : 32;
 
     /// This is the number of bits in an RK_Integer or RK_IntegerInline
     /// representation, which makes the number of entries in the list derivable.
-    unsigned integer_bitwidth;
+    unsigned integerBitwidth;
 
     /// This is the number of elements for an RK_Aggregate representation.
-    unsigned aggregate_numElements;
-  } aux;
+    unsigned aggregateNumElements;
+  } auxInfo;
 
 public:
   /// This enum is used to indicate the sort of value held by a SymbolicValue
@@ -238,7 +238,9 @@ class SymbolicValue {
 };
 
 static_assert(sizeof(SymbolicValue) == 2 * sizeof(void *),
-              "SymbolicValue should stay small and POD");
+              "SymbolicValue should stay small");
+static_assert(std::is_pod<SymbolicValue>::value,
+              "SymbolicValue should stay POD");
 
 inline llvm::raw_ostream &operator<<(llvm::raw_ostream &os, SymbolicValue val) {
   val.print(os);

lib/SIL/SILConstants.cpp

@@ -131,7 +131,7 @@ SymbolicValue SymbolicValue::getInteger(int64_t value, unsigned bitWidth) {
   SymbolicValue result;
   result.representationKind = RK_IntegerInline;
   result.value.integerInline = value;
-  result.aux.integer_bitwidth = bitWidth;
+  result.auxInfo.integerBitwidth = bitWidth;
   return result;
 }
 
@@ -150,28 +150,29 @@ SymbolicValue SymbolicValue::getInteger(const APInt &value,
   SymbolicValue result;
   result.representationKind = RK_Integer;
   result.value.integer = words;
-  result.aux.integer_bitwidth = value.getBitWidth();
+  result.auxInfo.integerBitwidth = value.getBitWidth();
   return result;
 }
 
 APInt SymbolicValue::getIntegerValue() const {
   assert(getKind() == Integer);
   if (representationKind == RK_IntegerInline) {
-    auto numBits = aux.integer_bitwidth;
+    auto numBits = auxInfo.integerBitwidth;
     return APInt(numBits, value.integerInline);
   }
 
   assert(representationKind == RK_Integer);
-  auto numBits = aux.integer_bitwidth;
-  auto numWords = (numBits + 63) / 64;
+  auto numBits = auxInfo.integerBitwidth;
+  auto numWords =
+      (numBits + APInt::APINT_BITS_PER_WORD - 1) / APInt::APINT_BITS_PER_WORD;
   return APInt(numBits, {value.integer, numWords});
 }
 
 unsigned SymbolicValue::getIntegerValueBitWidth() const {
   assert(getKind() == Integer);
   assert (representationKind == RK_IntegerInline ||
           representationKind == RK_Integer);
-  return aux.integer_bitwidth;
+  return auxInfo.integerBitwidth;
 }
 
 //===----------------------------------------------------------------------===//
@@ -190,13 +191,13 @@ SymbolicValue SymbolicValue::getAggregate(ArrayRef<SymbolicValue> elements,
   SymbolicValue result;
   result.representationKind = RK_Aggregate;
   result.value.aggregate = resultElts;
-  result.aux.aggregate_numElements = elements.size();
+  result.auxInfo.aggregateNumElements = elements.size();
   return result;
 }
 
 ArrayRef<SymbolicValue> SymbolicValue::getAggregateValue() const {
   assert(getKind() == Aggregate);
-  return ArrayRef<SymbolicValue>(value.aggregate, aux.aggregate_numElements);
+  return ArrayRef<SymbolicValue>(value.aggregate, auxInfo.aggregateNumElements);
 }
 
 //===----------------------------------------------------------------------===//
@@ -217,7 +218,7 @@ struct alignas(SourceLoc) UnknownSymbolicValue final
   UnknownReason reason;
 
   /// The number of elements in the call stack.
-  unsigned call_stack_size;
+  unsigned callStackSize;
 
   static UnknownSymbolicValue *create(SILNode *node, UnknownReason reason,
                                       ArrayRef<SourceLoc> elements,
@@ -235,20 +236,20 @@ struct alignas(SourceLoc) UnknownSymbolicValue final
   }
 
   ArrayRef<SourceLoc> getCallStack() const {
-    return {getTrailingObjects<SourceLoc>(), call_stack_size};
+    return {getTrailingObjects<SourceLoc>(), callStackSize};
   }
 
   // This is used by the llvm::TrailingObjects base class.
   size_t numTrailingObjects(OverloadToken<SourceLoc>) const {
-    return call_stack_size;
+    return callStackSize;
   }
 
 private:
   UnknownSymbolicValue() = delete;
   UnknownSymbolicValue(const UnknownSymbolicValue &) = delete;
   UnknownSymbolicValue(SILNode *node, UnknownReason reason,
-                       unsigned call_stack_size)
-      : node(node), reason(reason), call_stack_size(call_stack_size) {}
+                       unsigned callStackSize)
+      : node(node), reason(reason), callStackSize(callStackSize) {}
 };
 } // namespace swift
 
@@ -330,7 +331,7 @@ SymbolicValue SymbolicValue::lookThroughSingleElementAggregates() const {
 /// is an interesting SourceLoc to point at.
 /// Returns true if a diagnostic was emitted.
 static bool emitNoteDiagnostic(SILInstruction *badInst, UnknownReason reason,
-                               SILLocation fallbackLoc, std::string error) {
+                               SILLocation fallbackLoc) {
   auto loc = skipInternalLocations(badInst->getDebugLocation()).getLocation();
   if (loc.isNull()) {
     // If we have important clarifying information, make sure to emit it.
@@ -339,63 +340,55 @@ static bool emitNoteDiagnostic(SILInstruction *badInst, UnknownReason reason,
     loc = fallbackLoc;
   }
 
-  auto &module = badInst->getModule();
-  diagnose(module.getASTContext(), loc.getSourceLoc(),
-           diag::constexpr_unknown_reason, error)
-      .highlight(loc.getSourceRange());
-  return true;
-}
-
-/// Given that this is an 'Unknown' value, emit diagnostic notes providing
-/// context about what the problem is.
-void SymbolicValue::emitUnknownDiagnosticNotes(SILLocation fallbackLoc) {
-  auto badInst = dyn_cast<SILInstruction>(getUnknownNode());
-  if (!badInst)
-    return;
-
-  std::string error;
-  switch (getUnknownReason()) {
+  auto &ctx = badInst->getModule().getASTContext();
+  auto sourceLoc = loc.getSourceLoc();
+  switch (reason) {
   case UnknownReason::Default:
-    error = "could not fold operation";
+    diagnose(ctx, sourceLoc, diag::constexpr_unknown_reason_default)
+        .highlight(loc.getSourceRange());
     break;
   case UnknownReason::TooManyInstructions:
     // TODO: Should pop up a level of the stack trace.
-    error = "exceeded instruction limit: " + std::to_string(ConstExprLimit) +
-            " when evaluating the expression at compile time";
+    diagnose(ctx, sourceLoc, diag::constexpr_too_many_instructions,
+             ConstExprLimit)
+        .highlight(loc.getSourceRange());
     break;
   case UnknownReason::Loop:
-    error = "control flow loop found";
+    diagnose(ctx, sourceLoc, diag::constexpr_loop)
+        .highlight(loc.getSourceRange());
     break;
   case UnknownReason::Overflow:
-    error = "integer overflow detected";
+    diagnose(ctx, sourceLoc, diag::constexpr_overflow)
+        .highlight(loc.getSourceRange());
     break;
   case UnknownReason::Trap:
-    error = "trap detected";
+    diagnose(ctx, sourceLoc, diag::constexpr_trap)
+        .highlight(loc.getSourceRange());
     break;
   }
+  return true;
+}
+
+/// Given that this is an 'Unknown' value, emit diagnostic notes providing
+/// context about what the problem is.
+void SymbolicValue::emitUnknownDiagnosticNotes(SILLocation fallbackLoc) {
+  auto badInst = dyn_cast<SILInstruction>(getUnknownNode());
+  if (!badInst)
+    return;
 
-  bool emittedFirstNote =
-      emitNoteDiagnostic(badInst, getUnknownReason(), fallbackLoc, error);
+  bool emittedFirstNote = emitNoteDiagnostic(badInst, getUnknownReason(),
+                                             fallbackLoc);
 
   auto sourceLoc = fallbackLoc.getSourceLoc();
   auto &module = badInst->getModule();
   if (sourceLoc.isInvalid()) {
     diagnose(module.getASTContext(), sourceLoc, diag::constexpr_not_evaluable);
     return;
   }
-  auto &SM = module.getASTContext().SourceMgr;
-  unsigned originalDiagnosticLineNumber =
-      SM.getLineNumber(fallbackLoc.getSourceLoc());
   for (auto &sourceLoc : llvm::reverse(getUnknownCallStack())) {
     // Skip unknown sources.
     if (!sourceLoc.isValid())
       continue;
-    // Also skip notes that point to the same line as the original error, for
-    // example in:
-    //   #assert(foo(bar()))
-    // it is not useful to get three diagnostics referring to the same line.
-    if (SM.getLineNumber(sourceLoc) == originalDiagnosticLineNumber)
-      continue;
 
     auto diag = emittedFirstNote ? diag::constexpr_called_from
                                  : diag::constexpr_not_evaluable;

lib/SILOptimizer/Utils/ConstExpr.cpp

@@ -80,6 +80,10 @@ class ConstExprFunctionState {
         numInstEvaluated(numInstEvaluated) {}
 
   void setValue(SILValue value, SymbolicValue symVal) {
+    // TODO(constexpr patch): Uncomment this assertion once Address kinds have
+    // been added.
+    // assert(symVal.getKind() != SymbolicValue::Address &&
+    //        "calculatedValues does not hold addresses");
     calculatedValues.insert({value, symVal});
   }
 

test/SILOptimizer/pound_assert.sil

@@ -0,0 +1,31 @@
+// RUN: %target-sil-opt -enable-experimental-static-assert -assume-parsing-unqualified-ownership-sil %s -dataflow-diagnostics -verify
+
+sil_stage canonical
+
+import Builtin
+import Swift
+
+// Static assertion that "1 + 1 == 2".
+sil @test1 : $@convention(thin) () -> () {
+bb0:
+  %0 = integer_literal $Builtin.Int32, 1
+  %1 = builtin "add_Int32"(%0 : $Builtin.Int32, %0 : $Builtin.Int32) : $(Builtin.Int32)
+  %2 = integer_literal $Builtin.Int32, 2
+  %3 = builtin "cmp_eq_Int32"(%1 : $Builtin.Int32, %2 : $Builtin.Int32) : $(Builtin.Int1)
+  %4 = string_literal utf8 ""
+  %5 = builtin "poundAssert"(%3 : $Builtin.Int1, %4 : $Builtin.RawPointer) : $()
+  return undef : $()
+}
+
+// Static assertion that "2 + 2 == 5".
+sil @test2 : $@convention(thin) () -> () {
+bb0:
+  %0 = integer_literal $Builtin.Int32, 2
+  %1 = builtin "add_Int32"(%0 : $Builtin.Int32, %0 : $Builtin.Int32) : $(Builtin.Int32)
+  %2 = integer_literal $Builtin.Int32, 5
+  %3 = builtin "cmp_eq_Int32"(%1 : $Builtin.Int32, %2 : $Builtin.Int32) : $(Builtin.Int1)
+  %4 = string_literal utf8 ""
+  // expected-error @+1 {{assertion failed}}
+  %5 = builtin "poundAssert"(%3 : $Builtin.Int1, %4 : $Builtin.RawPointer) : $()
+  return undef : $()
+}

test/SILOptimizer/pound_assert.swift

@@ -34,7 +34,8 @@ func infiniteLoop() -> Int {
 }
 
 func test_infiniteLoop() {
-  // expected-error @+1 {{#assert condition not constant}}
+  // expected-error @+2 {{#assert condition not constant}}
+  // expected-note @+1 {{when called from here}}
   #assert(infiniteLoop() == 1)
 }
 

tools/sil-opt/SILOpt.cpp

@@ -214,6 +214,11 @@ static llvm::cl::opt<bool> DisableGuaranteedNormalArguments(
     llvm::cl::desc("Assume that the input module was compiled with "
                    "-disable-guaranteed-normal-arguments enabled"));
 
+static llvm::cl::opt<bool>
+EnableExperimentalStaticAssert(
+    "enable-experimental-static-assert", llvm::cl::Hidden,
+    llvm::cl::init(false), llvm::cl::desc("Enable experimental #assert"));
+
 /// Regular expression corresponding to the value given in one of the
 /// -pass-remarks* command line flags. Passes whose name matches this regexp
 /// will emit a diagnostic.
@@ -325,6 +330,9 @@ int main(int argc, char **argv) {
   Invocation.getLangOptions().OptimizationRemarkMissedPattern =
       createOptRemarkRegex(PassRemarksMissed);
 
+  Invocation.getLangOptions().EnableExperimentalStaticAssert =
+      EnableExperimentalStaticAssert;
+
   // Setup the SIL Options.
   SILOptions &SILOpts = Invocation.getSILOptions();
   SILOpts.InlineThreshold = SILInlineThreshold;