[ir] Improve validation speed with many failures

ReserveAdditional() was being used to grow the diagnostics vector by
only two slots at a time, which was triggering many copies of the
diagnostics vector for modules that have many failures. Instead just
let the vector grow naturally (which doubles the size when needed) and
manually get the reference to the error diagnostic.

Also add an early exit when iterating over instructions and errors
have already been found.

Fixed: 466695523
Change-Id: I8b862c8550237d29c75fa9d1a1aa150cf4fc86fa
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/280935
Commit-Queue: James Price <jrprice@google.com>
Reviewed-by: dan sinclair <dsinclair@chromium.org>

Author: jrprice

src/tint/lang/core/ir/validator.cc

@@ -1705,36 +1705,45 @@ void Validator::RunStructuralSoundnessChecks() {
 }
 
 diag::Diagnostic& Validator::AddError(const Instruction* inst) {
-    diagnostics_.ReserveAdditional(2);  // Ensure diagnostics don't resize alive after AddNote()
     auto src = Disassemble().InstructionSource(inst);
     auto& diag = AddError(src) << inst->FriendlyName() << ": ";
 
     if (!block_stack_.IsEmpty()) {
         AddNote(block_stack_.Back()) << "in block";
+
+        // Adding the note may trigger a resize and invalidate the error diagnostic reference, so we
+        // need to get a new reference to the error diagnostic here.
+        return *(diagnostics_.end() - 2);
     }
     return diag;
 }
 
 diag::Diagnostic& Validator::AddError(const Instruction* inst, size_t idx) {
-    diagnostics_.ReserveAdditional(2);  // Ensure diagnostics don't resize alive after AddNote()
     auto src =
         Disassemble().OperandSource(Disassembler::IndexedValue{inst, static_cast<uint32_t>(idx)});
     auto& diag = AddError(src) << inst->FriendlyName() << ": ";
 
     if (!block_stack_.IsEmpty()) {
         AddNote(block_stack_.Back()) << "in block";
+
+        // Adding the note may trigger a resize and invalidate the error diagnostic reference, so we
+        // need to get a new reference to the error diagnostic here.
+        return *(diagnostics_.end() - 2);
     }
     return diag;
 }
 
 diag::Diagnostic& Validator::AddResultError(const Instruction* inst, size_t idx) {
-    diagnostics_.ReserveAdditional(2);  // Ensure diagnostics don't resize alive after AddNote()
     auto src =
         Disassemble().ResultSource(Disassembler::IndexedValue{inst, static_cast<uint32_t>(idx)});
     auto& diag = AddError(src) << inst->FriendlyName() << ": ";
 
     if (!block_stack_.IsEmpty()) {
         AddNote(block_stack_.Back()) << "in block";
+
+        // Adding the note may trigger a resize and invalidate the error diagnostic reference, so we
+        // need to get a new reference to the error diagnostic here.
+        return *(diagnostics_.end() - 2);
     }
     return diag;
 }
@@ -3254,6 +3263,10 @@ void Validator::EndBlock() {
 }
 
 void Validator::QueueInstructions(const Instruction* inst) {
+    if (diagnostics_.ContainsErrors()) {
+        return;
+    }
+
     tasks_.Push([this, inst] {
         // Tasks are processed LIFO, so push the next instruction to the stack before checking the
         // current instruction, which may need to add more blocks to the stack itself.

src/tint/lang/core/ir/validator_flow_control_test.cc

@@ -974,17 +974,21 @@ TEST_F(IR_ValidatorTest, Continue_InLoopInit) {
     auto* f = b.Function("my_func", ty.void_());
     b.Append(f->Block(), [&] {
         auto* loop = b.Loop();
-        b.Append(loop->Initializer(), [&] { b.Continue(loop); });
+        b.Append(loop->Initializer(), [&]() {
+            auto* if_ = b.If(true);
+            b.Append(if_->True(), [&]() { b.Continue(loop); });
+            b.NextIteration(loop);
+        });
         b.Append(loop->Body(), [&] { b.ExitLoop(loop); });
         b.Return(f);
     });
 
     auto res = ir::Validate(mod);
     ASSERT_NE(res, Success);
     EXPECT_THAT(res.Failure().reason,
-                testing::HasSubstr(R"(:5:9 error: continue: must only be called from loop body
-        continue  # -> $B4
-        ^^^^^^^^
+                testing::HasSubstr(R"(:7:13 error: continue: must only be called from loop body
+            continue  # -> $B5
+            ^^^^^^^^
 )")) << res.Failure();
 }
 
@@ -1005,16 +1009,20 @@ TEST_F(IR_ValidatorTest, Continue_InLoopContinuing) {
     b.Append(f->Block(), [&] {
         auto* loop = b.Loop();
         b.Append(loop->Body(), [&] { b.ExitLoop(loop); });
-        b.Append(loop->Continuing(), [&] { b.Continue(loop); });
+        b.Append(loop->Continuing(), [&]() {
+            auto* if_ = b.If(true);
+            b.Append(if_->True(), [&]() { b.Continue(loop); });
+            b.NextIteration(loop);
+        });
         b.Return(f);
     });
 
     auto res = ir::Validate(mod);
     ASSERT_NE(res, Success);
     EXPECT_THAT(res.Failure().reason,
-                testing::HasSubstr(R"(:8:9 error: continue: must only be called from loop body
-        continue  # -> $B3
-        ^^^^^^^^
+                testing::HasSubstr(R"(:10:13 error: continue: must only be called from loop body
+            continue  # -> $B3
+            ^^^^^^^^
 )")) << res.Failure();
 }
 
@@ -1856,28 +1864,6 @@ TEST_F(IR_ValidatorTest, ExitLoop_InvalidJumpOverLoop) {
 TEST_F(IR_ValidatorTest, ExitLoop_InvalidInsideContinuing) {
     auto* loop = b.Loop();
 
-    loop->Continuing()->Append(b.ExitLoop(loop));
-    loop->Body()->Append(b.Continue(loop));
-
-    auto* f = b.Function("my_func", ty.void_());
-
-    b.Append(f->Block(), [&] {
-        b.Append(loop);
-        b.Return(f);
-    });
-
-    auto res = ir::Validate(mod);
-    ASSERT_NE(res, Success);
-    EXPECT_THAT(res.Failure().reason,
-                testing::HasSubstr(R"(:8:9 error: exit_loop: loop exit jumps out of continuing block
-        exit_loop  # loop_1
-        ^^^^^^^^^
-)")) << res.Failure();
-}
-
-TEST_F(IR_ValidatorTest, ExitLoop_InvalidInsideContinuingNested) {
-    auto* loop = b.Loop();
-
     b.Append(loop->Continuing(), [&]() {
         auto* if_ = b.If(true);
         b.Append(if_->True(), [&]() { b.ExitLoop(loop); });
@@ -1906,31 +1892,6 @@ TEST_F(IR_ValidatorTest, ExitLoop_InvalidInsideContinuingNested) {
 TEST_F(IR_ValidatorTest, ExitLoop_InvalidInsideInitializer) {
     auto* loop = b.Loop();
 
-    loop->Initializer()->Append(b.ExitLoop(loop));
-    loop->Continuing()->Append(b.NextIteration(loop));
-
-    b.Append(loop->Body(), [&] { b.Continue(loop); });
-
-    auto* f = b.Function("my_func", ty.void_());
-
-    b.Append(f->Block(), [&] {
-        b.Append(loop);
-        b.Return(f);
-    });
-
-    auto res = ir::Validate(mod);
-    ASSERT_NE(res, Success);
-    EXPECT_THAT(
-        res.Failure().reason,
-        testing::HasSubstr(R"(:5:9 error: exit_loop: loop exit not permitted in loop initializer
-        exit_loop  # loop_1
-        ^^^^^^^^^
-)")) << res.Failure();
-}
-
-TEST_F(IR_ValidatorTest, ExitLoop_InvalidInsideInitializerNested) {
-    auto* loop = b.Loop();
-
     b.Append(loop->Initializer(), [&]() {
         auto* if_ = b.If(true);
         b.Append(if_->True(), [&]() { b.ExitLoop(loop); });

src/tint/utils/diagnostic/diagnostic.h

@@ -86,8 +86,9 @@ class Diagnostic {
 /// List is a container of Diagnostic messages.
 class List {
   public:
-    /// The iterator type for this List
-    using iterator = VectorIterator<const Diagnostic>;
+    /// The iterator types for this List
+    using iterator = VectorIterator<Diagnostic>;
+    using const_iterator = VectorIterator<const Diagnostic>;
 
     /// Constructs the list with no elements.
     List();
@@ -186,11 +187,6 @@ class List {
         return Add(std::move(error));
     }
 
-    /// Ensures that the diagnostic list can fit an additional @p count diagnostics without
-    /// resizing. This is useful for ensuring that a reference returned by the AddX() methods is not
-    /// invalidated after another Add().
-    void ReserveAdditional(size_t count) { entries_.Reserve(entries_.Length() + count); }
-
     /// @returns true iff the diagnostic list contains errors diagnostics (or of
     /// higher severity).
     bool ContainsErrors() const { return error_count_ > 0; }
@@ -212,9 +208,13 @@ class List {
     /// @returns the number of entries in the list.
     size_t size() const { return entries_.Length(); }
     /// @returns the first diagnostic in the list.
-    iterator begin() const { return entries_.begin(); }
+    const_iterator begin() const { return entries_.begin(); }
+    /// @returns the last diagnostic in the list.
+    const_iterator end() const { return entries_.end(); }
+    /// @returns the first diagnostic in the list.
+    iterator begin() { return entries_.begin(); }
     /// @returns the last diagnostic in the list.
-    iterator end() const { return entries_.end(); }
+    iterator end() { return entries_.end(); }
 
   private:
     Vector<Diagnostic, 0> entries_;