Move initialization of the const mgr to the constructor.

The current code expects the users of the constant manager to initialize
it with all of the constants in the module.  The problem is that you do
not want to redo the work multiple times.  So I decided to move that
code to the constructor of the constant manager.  This way it will
always be initialized on first use.

I also removed an assert that expects all constant instructions to be
successfully mapped.  This is because not all OpConstant* instruction
can map to a constant, and neither do the OpSpecConstant* instructions.

The real problem is that an OpConstantComposite can contain a member
that is OpUndef.  I tried to treat OpUndef like OpConstantNull, but this
failed because an OpSpecConstantComposite with an OpUndef cannot be
changed to an OpConstantComposite.  Since I feel this case will not be
common, I decided to not complicate the code.

Fixes #1193.

diff --git a/source/opt/ccp_pass.cpp b/source/opt/ccp_pass.cpp
index ed633ca..077bf78 100644 (file)
--- a/source/opt/ccp_pass.cpp
+++ b/source/opt/ccp_pass.cpp
@@ -273,12 +273,8 @@ void CCPPass::Initialize(ir::IRContext* c) {
   // Populate the constant table with values from constant declarations in the
   // module.  The values of each OpConstant declaration is the identity
   // assignment (i.e., each constant is its own value).
-  for (const auto& inst : c->module()->GetConstants()) {
+  for (const auto& inst : context()->module()->GetConstants()) {
     values_[inst->result_id()] = inst->result_id();
-    if (!const_mgr_->MapInst(inst)) {
-      assert(false &&
-             "Could not map a new constant value to its defining instruction");
-    }
   }
 }
 

diff --git a/source/opt/constants.cpp b/source/opt/constants.cpp
index d4224d2..080f048 100644 (file)
--- a/source/opt/constants.cpp
+++ b/source/opt/constants.cpp
@@ -22,6 +22,15 @@ namespace spvtools {
 namespace opt {
 namespace analysis {
 
+ConstantManager::ConstantManager(ir::IRContext* ctx) : ctx_(ctx) {
+  // Populate the constant table with values from constant declarations in the
+  // module.  The values of each OpConstant declaration is the identity
+  // assignment (i.e., each constant is its own value).
+  for (const auto& inst : ctx_->module()->GetConstants()) {
+    MapInst(inst);
+  }
+}
+
 Type* ConstantManager::GetType(const ir::Instruction* inst) const {
   return context()->get_type_mgr()->GetType(inst->type_id());
 }

diff --git a/source/opt/constants.h b/source/opt/constants.h
index 70d4c5d..bf775a5 100644 (file)
--- a/source/opt/constants.h
+++ b/source/opt/constants.h
@@ -343,7 +343,7 @@ struct ConstantEqual {
 // This class represents a pool of constants.
 class ConstantManager {
  public:
-  ConstantManager(ir::IRContext* ctx) : ctx_(ctx) {}
+  ConstantManager(ir::IRContext* ctx);
 
   ir::IRContext* context() const { return ctx_; }
 

diff --git a/source/opt/instruction.h b/source/opt/instruction.h
index 30b3d78..27a6cf9 100644 (file)
--- a/source/opt/instruction.h
+++ b/source/opt/instruction.h
@@ -25,6 +25,7 @@
 #include "util/ilist_node.h"
 
 #include "latest_version_spirv_header.h"
+#include "reflect.h"
 #include "spirv-tools/libspirv.h"
 
 namespace spvtools {
@@ -567,10 +568,7 @@ bool Instruction::IsLoad() const { return spvOpcodeIsLoad(opcode()); }
 
 bool Instruction::IsAtomicOp() const { return spvOpcodeIsAtomicOp(opcode()); }
 
-bool Instruction::IsConstant() const {
-  return spvOpcodeIsConstant(opcode()) &&
-         !spvOpcodeIsScalarSpecConstant(opcode());
-}
+bool Instruction::IsConstant() const { return IsConstantInst(opcode()); }
 }  // namespace ir
 }  // namespace spvtools
 

diff --git a/test/opt/ccp_test.cpp b/test/opt/ccp_test.cpp
index 24ffe34..6f3122e 100644 (file)
--- a/test/opt/ccp_test.cpp
+++ b/test/opt/ccp_test.cpp
@@ -523,6 +523,37 @@ TEST_F(CCPTest, LoopInductionVariables) {
   SinglePassRunAndMatch<opt::CCPPass>(spv_asm, true);
 }
 
+TEST_F(CCPTest, HandleCompositeWithUndef) {
+  // Check to make sure that CCP does not crash when given a "constant" struct
+  // with an undef.  If at a later time CCP is enhanced to optimize this case,
+  // it is not wrong.
+  const std::string spv_asm = R"(
+               OpCapability Shader
+          %1 = OpExtInstImport "GLSL.std.450"
+               OpMemoryModel Logical GLSL450
+               OpEntryPoint Fragment %main "main"
+               OpExecutionMode %main OriginUpperLeft
+               OpSource HLSL 500
+               OpName %main "main"
+       %void = OpTypeVoid
+          %4 = OpTypeFunction %void
+        %int = OpTypeInt 32 1
+       %bool = OpTypeBool
+  %_struct_7 = OpTypeStruct %int %int
+      %int_1 = OpConstant %int 1
+          %9 = OpUndef %int
+         %10 = OpConstantComposite %_struct_7 %int_1 %9
+       %main = OpFunction %void None %4
+         %11 = OpLabel
+         %12 = OpCompositeExtract %int %10 0
+         %13 = OpCopyObject %int %12
+               OpReturn
+               OpFunctionEnd
+  )";
+
+  auto res = SinglePassRunToBinary<opt::CCPPass>(spv_asm, true);
+  EXPECT_EQ(std::get<1>(res), opt::Pass::Status::SuccessWithoutChange);
+}
 #endif
 
 }  // namespace