From ed96301c6c4ab90c8dcdf5f4a11bec72854a5f95 Mon Sep 17 00:00:00 2001
From: Alastair Donaldson <afdx@google.com>
Date: Wed, 15 Apr 2020 11:39:33 +0100
Subject: [PATCH] spirv-fuzz: Fix to outliner (#3302)

Adds an extra condition on when a region can be outlined to avoid the
case where a region ends with a loop head but such that the loop's
continue target is in the region.  (Outlining such a region would mean
that the loop merge is in the original function and the continue target
in the outlined function.)
---
 .../fuzz/transformation_outline_function.cpp  | 21 +++++--
 .../transformation_outline_function_test.cpp  | 61 ++++++++++++++++++-
 2 files changed, 77 insertions(+), 5 deletions(-)

diff --git a/source/fuzz/transformation_outline_function.cpp b/source/fuzz/transformation_outline_function.cpp
index 39a4b01420..d84545a297 100644
--- a/source/fuzz/transformation_outline_function.cpp
+++ b/source/fuzz/transformation_outline_function.cpp
@@ -198,10 +198,23 @@ bool TransformationOutlineFunction::IsApplicable(
   for (auto& block : *entry_block->GetParent()) {
     if (&block == exit_block) {
       // It is OK (and typically expected) for the exit block of the region to
-      // have successors outside the region.  It is also OK for the exit block
-      // to head a structured control flow construct - the block containing the
-      // call to the outlined function will end up heading this construct if
-      // outlining takes place.
+      // have successors outside the region.
+      //
+      // It is also OK for the exit block to head a structured control flow
+      // construct - the block containing the call to the outlined function will
+      // end up heading this construct if outlining takes place.  However, we
+      // must ensure that if the exit block heads a loop, the continue target
+      // for this loop is outside the region.
+      if (auto loop_merge = block.GetLoopMergeInst()) {
+        // The exit block heads a loop
+        auto continue_target =
+            ir_context->cfg()->block(loop_merge->GetSingleWordOperand(1));
+        if (region_set.count(continue_target)) {
+          // The continue target for the loop is in the region.
+          return false;
+        }
+      }
+
       continue;
     }
 
diff --git a/test/fuzz/transformation_outline_function_test.cpp b/test/fuzz/transformation_outline_function_test.cpp
index 4b0f5ea7ec..01bba317a3 100644
--- a/test/fuzz/transformation_outline_function_test.cpp
+++ b/test/fuzz/transformation_outline_function_test.cpp
@@ -2465,7 +2465,6 @@ TEST(TransformationOutlineFunctionTest,
   // This checks that we cannot outline a region of code if it produces a
   // pointer result id that gets used outside the region.  This avoids creating
   // a struct with a pointer member.
-
   std::string shader = R"(
                OpCapability Shader
           %1 = OpExtInstImport "GLSL.std.450"
@@ -2520,6 +2519,66 @@ TEST(TransformationOutlineFunctionTest,
       transformation.IsApplicable(context.get(), transformation_context));
 }
 
+TEST(TransformationOutlineFunctionTest,
+     ExitBlockHeadsLoopButContinueConstructIsInRegion) {
+  // This checks that it is not possible outline a region that ends in a loop
+  // head if the continue target for the loop is inside the region.
+  std::string shader = R"(
+               OpCapability Shader
+          %1 = OpExtInstImport "GLSL.std.450"
+               OpMemoryModel Logical GLSL450
+               OpEntryPoint Fragment %4 "main"
+               OpExecutionMode %4 OriginUpperLeft
+               OpSource ESSL 310
+          %2 = OpTypeVoid
+          %3 = OpTypeFunction %2
+         %15 = OpTypeInt 32 1
+         %35 = OpTypeBool
+         %39 = OpConstant %15 1
+         %40 = OpConstantTrue %35
+          %4 = OpFunction %2 None %3
+          %5 = OpLabel
+               OpBranch %22
+         %22 = OpLabel
+               OpBranch %23
+         %23 = OpLabel
+         %24 = OpPhi %15 %39 %22 %39 %25
+               OpLoopMerge %26 %25 None
+               OpBranchConditional %40 %25 %26
+         %25 = OpLabel
+               OpBranch %23
+         %26 = OpLabel
+               OpReturn
+               OpFunctionEnd
+  )";
+
+  const auto env = SPV_ENV_UNIVERSAL_1_5;
+  const auto consumer = nullptr;
+  const auto context = BuildModule(env, consumer, shader, kFuzzAssembleOption);
+  ASSERT_TRUE(IsValid(env, context.get()));
+
+  FactManager fact_manager;
+  spvtools::ValidatorOptions validator_options;
+  TransformationContext transformation_context(&fact_manager,
+                                               validator_options);
+
+  TransformationOutlineFunction transformation(
+      /*entry_block*/ 22,
+      /*exit_block*/ 23,
+      /*new_function_struct_return_type_id*/ 200,
+      /*new_function_type_id*/ 201,
+      /*new_function_id*/ 202,
+      /*new_function_region_entry_block*/ 203,
+      /*new_caller_result_id*/ 204,
+      /*new_callee_result_id*/ 205,
+      /*input_id_to_fresh_id*/ {},
+      /*output_id_to_fresh_id*/ {});
+
+  ASSERT_FALSE(
+      transformation.IsApplicable(context.get(), transformation_context));
+  transformation.Apply(context.get(), &transformation_context);
+}
+
 TEST(TransformationOutlineFunctionTest, Miscellaneous1) {
   // This tests outlining of some non-trivial code.