From effafedcee7310c50bf5e93c9d4bebc3b1bee49a Mon Sep 17 00:00:00 2001 From: dan sinclair Date: Fri, 20 Jul 2018 11:09:30 -0400 Subject: [PATCH] Replace opt::Instruction type and result cache with flags. (#1718) Currentlty opt::Instruction class holds a cache of the result_id and type_id for the instruction. That cache needs to be updated if the underlying operand values are changes. This CL changes the cache to being a flag if there is a type or result id for the instruction. We then retrieve the value if needed from the operands. --- source/opt/instruction.cpp | 36 +++++++++++------------ source/opt/instruction.h | 60 +++++++++++++++++++++++++------------- 2 files changed, 57 insertions(+), 39 deletions(-) diff --git a/source/opt/instruction.cpp b/source/opt/instruction.cpp index 12026846d..ea75657fd 100644 --- a/source/opt/instruction.cpp +++ b/source/opt/instruction.cpp @@ -36,24 +36,24 @@ Instruction::Instruction(IRContext* c) : utils::IntrusiveNodeBase(), context_(c), opcode_(SpvOpNop), - type_id_(0), - result_id_(0), + has_type_id_(false), + has_result_id_(false), unique_id_(c->TakeNextUniqueId()) {} Instruction::Instruction(IRContext* c, SpvOp op) : utils::IntrusiveNodeBase(), context_(c), opcode_(op), - type_id_(0), - result_id_(0), + has_type_id_(false), + has_result_id_(false), unique_id_(c->TakeNextUniqueId()) {} Instruction::Instruction(IRContext* c, const spv_parsed_instruction_t& inst, std::vector&& dbg_line) : context_(c), opcode_(static_cast(inst.opcode)), - type_id_(inst.type_id), - result_id_(inst.result_id), + has_type_id_(inst.type_id != 0), + has_result_id_(inst.result_id != 0), unique_id_(c->TakeNextUniqueId()), dbg_line_insts_(std::move(dbg_line)) { assert((!IsDebugLineInst(opcode_) || dbg_line.empty()) && @@ -72,17 +72,17 @@ Instruction::Instruction(IRContext* c, SpvOp op, uint32_t ty_id, : utils::IntrusiveNodeBase(), context_(c), opcode_(op), - type_id_(ty_id), - result_id_(res_id), + has_type_id_(ty_id != 0), + has_result_id_(res_id != 0), unique_id_(c->TakeNextUniqueId()), operands_() { - if (type_id_ != 0) { + if (has_type_id_) { operands_.emplace_back(spv_operand_type_t::SPV_OPERAND_TYPE_TYPE_ID, - std::initializer_list{type_id_}); + std::initializer_list{ty_id}); } - if (result_id_ != 0) { + if (has_result_id_) { operands_.emplace_back(spv_operand_type_t::SPV_OPERAND_TYPE_RESULT_ID, - std::initializer_list{result_id_}); + std::initializer_list{res_id}); } operands_.insert(operands_.end(), in_operands.begin(), in_operands.end()); } @@ -90,16 +90,16 @@ Instruction::Instruction(IRContext* c, SpvOp op, uint32_t ty_id, Instruction::Instruction(Instruction&& that) : utils::IntrusiveNodeBase(), opcode_(that.opcode_), - type_id_(that.type_id_), - result_id_(that.result_id_), + has_type_id_(that.has_type_id_), + has_result_id_(that.has_result_id_), unique_id_(that.unique_id_), operands_(std::move(that.operands_)), dbg_line_insts_(std::move(that.dbg_line_insts_)) {} Instruction& Instruction::operator=(Instruction&& that) { opcode_ = that.opcode_; - type_id_ = that.type_id_; - result_id_ = that.result_id_; + has_type_id_ = that.has_type_id_; + has_result_id_ = that.has_result_id_; unique_id_ = that.unique_id_; operands_ = std::move(that.operands_); dbg_line_insts_ = std::move(that.dbg_line_insts_); @@ -109,8 +109,8 @@ Instruction& Instruction::operator=(Instruction&& that) { Instruction* Instruction::Clone(IRContext* c) const { Instruction* clone = new Instruction(c); clone->opcode_ = opcode_; - clone->type_id_ = type_id_; - clone->result_id_ = result_id_; + clone->has_type_id_ = has_type_id_; + clone->has_result_id_ = has_result_id_; clone->unique_id_ = c->TakeNextUniqueId(); clone->operands_ = operands_; clone->dbg_line_insts_ = dbg_line_insts_; diff --git a/source/opt/instruction.h b/source/opt/instruction.h index e6528ee5f..105c4a328 100644 --- a/source/opt/instruction.h +++ b/source/opt/instruction.h @@ -107,8 +107,8 @@ class Instruction : public utils::IntrusiveNodeBase { : utils::IntrusiveNodeBase(), context_(nullptr), opcode_(SpvOpNop), - type_id_(0), - result_id_(0), + has_type_id_(false), + has_result_id_(false), unique_id_(0) {} // Creates a default OpNop instruction. @@ -153,8 +153,12 @@ class Instruction : public utils::IntrusiveNodeBase { // TODO(qining): Remove this function when instruction building and insertion // is well implemented. void SetOpcode(SpvOp op) { opcode_ = op; } - uint32_t type_id() const { return type_id_; } - uint32_t result_id() const { return result_id_; } + uint32_t type_id() const { + return has_type_id_ ? GetSingleWordOperand(0) : 0; + } + uint32_t result_id() const { + return has_result_id_ ? GetSingleWordOperand(has_type_id_ ? 1 : 0) : 0; + } uint32_t unique_id() const { assert(unique_id_ != 0); return unique_id_; @@ -211,7 +215,7 @@ class Instruction : public utils::IntrusiveNodeBase { inline void SetResultType(uint32_t ty_id); // Sets the result id inline void SetResultId(uint32_t res_id); - inline bool HasResultId() const { return result_id_ != 0; } + inline bool HasResultId() const { return has_result_id_; } // Remove the |index|-th operand void RemoveOperand(uint32_t index) { operands_.erase(operands_.begin() + index); @@ -424,7 +428,9 @@ class Instruction : public utils::IntrusiveNodeBase { private: // Returns the total count of result type id and result id. uint32_t TypeResultIdCount() const { - return (type_id_ != 0) + (result_id_ != 0); + if (has_type_id_ && has_result_id_) return 2; + if (has_type_id_ || has_result_id_) return 1; + return 0; } // Returns true if the instruction declares a variable that is read-only. The @@ -439,8 +445,8 @@ class Instruction : public utils::IntrusiveNodeBase { IRContext* context_; // IR Context SpvOp opcode_; // Opcode - uint32_t type_id_; // Result type id. A value of 0 means no result type id. - uint32_t result_id_; // Result id. A value of 0 means no result id. + bool has_type_id_; // True if the instruction has a type id + bool has_result_id_; // True if the instruction has a result id uint32_t unique_id_; // Unique instruction id // All logical operands, including result type id and result id. OperandList operands_; @@ -506,28 +512,43 @@ inline void Instruction::SetInOperands(OperandList&& new_operands) { } inline void Instruction::SetResultId(uint32_t res_id) { - result_id_ = res_id; - auto ridx = (type_id_ != 0) ? 1 : 0; - assert(operands_[ridx].type == SPV_OPERAND_TYPE_RESULT_ID); + // TODO(dsinclair): Allow setting a result id if there wasn't one + // previously. Need to make room in the operands_ array to place the result, + // and update the has_result_id_ flag. + assert(has_result_id_); + + // TODO(dsinclair): Allow removing the result id. This needs to make sure, + // if there was a result id previously to remove it from the operands_ array + // and reset the has_result_id_ flag. + assert(res_id != 0); + + auto ridx = has_type_id_ ? 1 : 0; operands_[ridx].words = {res_id}; } inline void Instruction::SetResultType(uint32_t ty_id) { - if (type_id_ != 0) { - type_id_ = ty_id; - assert(operands_.front().type == SPV_OPERAND_TYPE_TYPE_ID); - operands_.front().words = {ty_id}; - } + // TODO(dsinclair): Allow setting a type id if there wasn't one + // previously. Need to make room in the operands_ array to place the result, + // and update the has_type_id_ flag. + assert(has_type_id_); + + // TODO(dsinclair): Allow removing the type id. This needs to make sure, + // if there was a type id previously to remove it from the operands_ array + // and reset the has_type_id_ flag. + assert(ty_id != 0); + + operands_.front().words = {ty_id}; } inline bool Instruction::IsNop() const { - return opcode_ == SpvOpNop && type_id_ == 0 && result_id_ == 0 && + return opcode_ == SpvOpNop && !has_type_id_ && !has_result_id_ && operands_.empty(); } inline void Instruction::ToNop() { opcode_ = SpvOpNop; - type_id_ = result_id_ = 0; + has_type_id_ = false; + has_result_id_ = false; operands_.clear(); } @@ -576,9 +597,6 @@ inline void Instruction::ForEachInst( inline void Instruction::ForEachId(const std::function& f) { for (auto& opnd : operands_) if (spvIsIdType(opnd.type)) f(&opnd.words[0]); - if (type_id_ != 0u) type_id_ = GetSingleWordOperand(0u); - if (result_id_ != 0u) - result_id_ = GetSingleWordOperand(type_id_ == 0u ? 0u : 1u); } inline void Instruction::ForEachId(