improve debug output, finalize explicit constructor refactor, and begin improving diagnostic messages

Author: jturcotti

include/swift/SILOptimizer/Utils/PartitionUtils.h

@@ -79,14 +79,22 @@ class PartitionOp {
   // generated during compilation from a SILBasicBlock
   SILInstruction *sourceInst;
 
+  // Record an AST expression corresponding to this PartitionOp, currently
+  // populated only for Consume expressions to indicate the value being consumed
+  Expr *sourceExpr;
+
   // TODO: can the following declarations be merged?
   PartitionOp(PartitionOpKind OpKind, Element arg1,
-              SILInstruction *sourceInst = nullptr)
-      : OpKind(OpKind), OpArgs({arg1}), sourceInst(sourceInst) {}
+              SILInstruction *sourceInst = nullptr,
+              Expr* sourceExpr = nullptr)
+      : OpKind(OpKind), OpArgs({arg1}),
+        sourceInst(sourceInst), sourceExpr(sourceExpr) {}
 
   PartitionOp(PartitionOpKind OpKind, Element arg1, Element arg2,
-              SILInstruction *sourceInst = nullptr)
-      : OpKind(OpKind), OpArgs({arg1, arg2}), sourceInst(sourceInst) {}
+              SILInstruction *sourceInst = nullptr,
+              Expr* sourceExpr = nullptr)
+      : OpKind(OpKind), OpArgs({arg1, arg2}),
+        sourceInst(sourceInst), sourceExpr(sourceExpr) {}
 
   friend class Partition;
 
@@ -102,8 +110,10 @@ class PartitionOp {
   }
 
   static PartitionOp Consume(Element tgt,
-                             SILInstruction *sourceInst = nullptr) {
-    return PartitionOp(PartitionOpKind::Consume, tgt, sourceInst);
+                             SILInstruction *sourceInst = nullptr,
+                             Expr *sourceExpr = nullptr) {
+    return PartitionOp(PartitionOpKind::Consume, tgt,
+                       sourceInst, sourceExpr);
   }
 
   static PartitionOp Merge(Element tgt1, Element tgt2,
@@ -116,7 +126,9 @@ class PartitionOp {
     return PartitionOp(PartitionOpKind::Require, tgt, sourceInst);
   }
 
-  bool operator==(const PartitionOp &other) const = default;
+  bool operator==(const PartitionOp &other) const {
+      return OpKind == other.OpKind && sourceInst == other.sourceInst;
+  };
   // implemented for insertion into std::map
   bool operator<(const PartitionOp &other) const {
     if (OpKind != other.OpKind)

lib/SILOptimizer/Mandatory/SendNonSendable.cpp

@@ -592,19 +592,26 @@ class PartitionOpTranslator {
 
     //translate each SIL instruction to a PartitionOp, if necessary
     std::vector<PartitionOp> partitionOps;
+    int lastTranslationIndex = -1;
     for (SILInstruction &instruction : *basicBlock) {
       auto ops = translateSILInstruction(&instruction);
       for (PartitionOp &op : ops) {
         partitionOps.push_back(op);
 
         LLVM_DEBUG(
-            llvm::dbgs() << " ┌─┬─╼";
-            instruction.dump();
-            llvm::dbgs() << " │ └─╼  ";
-            instruction.getLoc().getSourceLoc().printLineAndColumn(llvm::dbgs(), function->getASTContext().SourceMgr);
-            llvm::dbgs() << " │ translation #" << translationIndex;
-            llvm::dbgs() << "\n └─────╼ ";
+            if (translationIndex != lastTranslationIndex) {
+              llvm::dbgs() << " ┌─┬─╼";
+              instruction.dump();
+              llvm::dbgs() << " │ └─╼  ";
+              instruction.getLoc().getSourceLoc().printLineAndColumn(
+                  llvm::dbgs(), function->getASTContext().SourceMgr);
+              llvm::dbgs() << " │ translation #" << translationIndex;
+              llvm::dbgs() << "\n └─────╼ ";
+            } else {
+              llvm::dbgs() << "      └╼ ";
+            }
             op.dump();
+            lastTranslationIndex = translationIndex;
         );
       }
     }
@@ -617,7 +624,7 @@ class PartitionOpTranslator {
 // SILBasicBlock for the region-based Sendable checking fixpoint analysis.
 // In particular, it records flags such as whether the block has been
 // reached by the analysis, whether the prior round indicated that this block
-// needs to be updated; it recorsd aux data such as the underlying basic block
+// needs to be updated; it records aux data such as the underlying basic block
 // and associated PartitionOpTranslator; and most importantly of all it includes
 // region partitions at entry and exit to this block - these are the stateful
 // component of the fixpoint analysis.
@@ -1218,8 +1225,13 @@ class PartitionAnalysis {
           /*handleFailure=*/
           [&](const PartitionOp& partitionOp, TrackableValueID consumedVal) {
             auto expr = getExprForPartitionOp(partitionOp);
+
+            // ensure that multiple consumptions at the same AST node are only
+            // entered once into the race tracer
             if (hasBeenEmitted(expr)) return;
+
             raceTracer.traceUseOfConsumedValue(partitionOp, consumedVal);
+
             /*
              * This handles diagnosing accesses to consumed values at the site
              * of access instead of the site of consumption, as this is less
@@ -1229,6 +1241,7 @@ class PartitionAnalysis {
                 expr->getLoc(), diag::consumed_value_used);
                 */
           },
+
           /*handleConsumeNonConsumable=*/
           [&](const PartitionOp& partitionOp, TrackableValueID consumedVal) {
             auto expr = getExprForPartitionOp(partitionOp);
@@ -1247,11 +1260,13 @@ class PartitionAnalysis {
               expr->getLoc(), diag::consumption_yields_race,
               numDisplayed, numDisplayed != 1, numHidden > 0, numHidden);
         },
+
         /*diagnoseRequire=*/
         [&](const PartitionOp& requireOp) {
           auto expr = getExprForPartitionOp(requireOp);
           function->getASTContext().Diags.diagnose(
-              expr->getLoc(), diag::possible_racy_access_site);
+              expr->getLoc(), diag::possible_racy_access_site)
+              .highlight(expr->getSourceRange());
         });
   }