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.
This commit is contained in:
Steven Perron 2018-01-12 11:23:57 -05:00
parent 86dec646f5
commit 24f9947050
5 changed files with 44 additions and 10 deletions

View File

@ -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");
}
}
}

View File

@ -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());
}

View File

@ -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_; }

View File

@ -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

View File

@ -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