[turbofan] Cleanup duplicated/unused code in InstructionSelector.

- Use C++11 range based for loops.
- Remove duplicated virtual register set in unittests.
- Don't expose implementation details of InstructionSelector.

TEST=unittests
R=dcarney@chromium.org

Review URL: https://codereview.chromium.org/837423002

Cr-Commit-Position: refs/heads/master@{#25997}

src/compiler/instruction-selector.cc

@@ -4,7 +4,6 @@
 
 #include "src/compiler/instruction-selector.h"
 
-#include "src/compiler/graph.h"
 #include "src/compiler/instruction-selector-impl.h"
 #include "src/compiler/node-matchers.h"
 #include "src/compiler/node-properties-inl.h"
@@ -14,57 +13,56 @@ namespace v8 {
 namespace internal {
 namespace compiler {
 
-InstructionSelector::InstructionSelector(Zone* local_zone, Graph* graph,
+InstructionSelector::InstructionSelector(Zone* zone, size_t node_count,
                                          Linkage* linkage,
                                          InstructionSequence* sequence,
                                          Schedule* schedule,
                                          SourcePositionTable* source_positions,
                                          Features features)
-    : zone_(local_zone),
+    : zone_(zone),
       linkage_(linkage),
       sequence_(sequence),
       source_positions_(source_positions),
       features_(features),
       schedule_(schedule),
-      node_map_(graph->NodeCount(), kNodeUnmapped, zone()),
       current_block_(NULL),
-      instructions_(zone()),
-      defined_(graph->NodeCount(), false, zone()),
-      used_(graph->NodeCount(), false, zone()) {}
+      instructions_(zone),
+      defined_(node_count, false, zone),
+      used_(node_count, false, zone),
+      virtual_registers_(node_count,
+                         UnallocatedOperand::kInvalidVirtualRegister, zone) {
+  instructions_.reserve(node_count);
+}
 
 
 void InstructionSelector::SelectInstructions() {
   // Mark the inputs of all phis in loop headers as used.
   BasicBlockVector* blocks = schedule()->rpo_order();
-  for (BasicBlockVectorIter i = blocks->begin(); i != blocks->end(); ++i) {
-    BasicBlock* block = *i;
+  for (auto const block : *blocks) {
     if (!block->IsLoopHeader()) continue;
-    DCHECK_NE(0, static_cast<int>(block->PredecessorCount()));
-    DCHECK_NE(1, static_cast<int>(block->PredecessorCount()));
-    for (BasicBlock::const_iterator j = block->begin(); j != block->end();
-         ++j) {
-      Node* phi = *j;
+    DCHECK_LE(2, block->PredecessorCount());
+    for (Node* const phi : *block) {
       if (phi->opcode() != IrOpcode::kPhi) continue;
 
       // Mark all inputs as used.
-      for (Node* const k : phi->inputs()) {
-        MarkAsUsed(k);
+      for (Node* const input : phi->inputs()) {
+        MarkAsUsed(input);
       }
     }
   }
 
   // Visit each basic block in post order.
-  for (BasicBlockVectorRIter i = blocks->rbegin(); i != blocks->rend(); ++i) {
+  for (auto i = blocks->rbegin(); i != blocks->rend(); ++i) {
     VisitBlock(*i);
   }
 
   // Schedule the selected instructions.
-  for (BasicBlockVectorIter i = blocks->begin(); i != blocks->end(); ++i) {
-    BasicBlock* block = *i;
+  for (auto const block : *blocks) {
     InstructionBlock* instruction_block =
         sequence()->InstructionBlockAt(block->GetRpoNumber());
     size_t end = instruction_block->code_end();
     size_t start = instruction_block->code_start();
+    DCHECK_LE(end, start);
     sequence()->StartBlock(block->GetRpoNumber());
     while (start-- > end) {
       sequence()->AddInstruction(instructions_[start]);
@@ -180,58 +178,70 @@ bool InstructionSelector::CanCover(Node* user, Node* node) const {
 
 
 int InstructionSelector::GetVirtualRegister(const Node* node) {
-  if (node_map_[node->id()] == kNodeUnmapped) {
-    node_map_[node->id()] = sequence()->NextVirtualRegister();
+  DCHECK_NOT_NULL(node);
+  size_t const id = node->id();
+  DCHECK_LT(id, virtual_registers_.size());
+  int virtual_register = virtual_registers_[id];
+  if (virtual_register == UnallocatedOperand::kInvalidVirtualRegister) {
+    virtual_register = sequence()->NextVirtualRegister();
+    virtual_registers_[id] = virtual_register;
   }
-  return node_map_[node->id()];
+  return virtual_register;
 }
 
 
-int InstructionSelector::GetMappedVirtualRegister(const Node* node) const {
-  return node_map_[node->id()];
+const std::map<NodeId, int> InstructionSelector::GetVirtualRegistersForTesting()
+    const {
+  std::map<NodeId, int> virtual_registers;
+  for (size_t n = 0; n < virtual_registers_.size(); ++n) {
+    if (virtual_registers_[n] != UnallocatedOperand::kInvalidVirtualRegister) {
+      NodeId const id = static_cast<NodeId>(n);
+      virtual_registers.insert(std::make_pair(id, virtual_registers_[n]));
+    }
+  }
+  return virtual_registers;
 }
 
 
 bool InstructionSelector::IsDefined(Node* node) const {
   DCHECK_NOT_NULL(node);
-  NodeId id = node->id();
-  DCHECK(id >= 0);
-  DCHECK(id < static_cast<NodeId>(defined_.size()));
+  size_t const id = node->id();
+  DCHECK_LT(id, defined_.size());
   return defined_[id];
 }
 
 
 void InstructionSelector::MarkAsDefined(Node* node) {
   DCHECK_NOT_NULL(node);
-  NodeId id = node->id();
-  DCHECK(id >= 0);
-  DCHECK(id < static_cast<NodeId>(defined_.size()));
+  size_t const id = node->id();
+  DCHECK_LT(id, defined_.size());
   defined_[id] = true;
 }
 
 
 bool InstructionSelector::IsUsed(Node* node) const {
+  DCHECK_NOT_NULL(node);
   if (!node->op()->HasProperty(Operator::kEliminatable)) return true;
-  NodeId id = node->id();
-  DCHECK(id >= 0);
-  DCHECK(id < static_cast<NodeId>(used_.size()));
+  size_t const id = node->id();
+  DCHECK_LT(id, used_.size());
   return used_[id];
 }
 
 
 void InstructionSelector::MarkAsUsed(Node* node) {
   DCHECK_NOT_NULL(node);
-  NodeId id = node->id();
-  DCHECK(id >= 0);
-  DCHECK(id < static_cast<NodeId>(used_.size()));
+  size_t const id = node->id();
+  DCHECK_LT(id, used_.size());
   used_[id] = true;
 }
 
 
 bool InstructionSelector::IsDouble(const Node* node) const {
   DCHECK_NOT_NULL(node);
-  int virtual_register = GetMappedVirtualRegister(node);
-  if (virtual_register == kNodeUnmapped) return false;
+  int const virtual_register = virtual_registers_[node->id()];
+  if (virtual_register == UnallocatedOperand::kInvalidVirtualRegister) {
+    return false;
+  }
   return sequence()->IsDouble(virtual_register);
 }
 
@@ -245,8 +255,10 @@ void InstructionSelector::MarkAsDouble(Node* node) {
 
 bool InstructionSelector::IsReference(const Node* node) const {
   DCHECK_NOT_NULL(node);
-  int virtual_register = GetMappedVirtualRegister(node);
-  if (virtual_register == kNodeUnmapped) return false;
+  int const virtual_register = virtual_registers_[node->id()];
+  if (virtual_register == UnallocatedOperand::kInvalidVirtualRegister) {
+    return false;
+  }
   return sequence()->IsReference(virtual_register);
 }
 

src/compiler/instruction-selector.h

@@ -5,7 +5,7 @@
 #ifndef V8_COMPILER_INSTRUCTION_SELECTOR_H_
 #define V8_COMPILER_INSTRUCTION_SELECTOR_H_
 
-#include <deque>
+#include <map>
 
 #include "src/compiler/common-operator.h"
 #include "src/compiler/instruction.h"
@@ -21,17 +21,12 @@ struct CallBuffer;  // TODO(bmeurer): Remove this.
 class FlagsContinuation;
 class Linkage;
 
-typedef IntVector NodeToVregMap;
-
 class InstructionSelector FINAL {
  public:
-  static const int kNodeUnmapped = -1;
-
   // Forward declarations.
   class Features;
 
-  // TODO(dcarney): pass in vreg mapping instead of graph.
-  InstructionSelector(Zone* local_zone, Graph* graph, Linkage* linkage,
+  InstructionSelector(Zone* zone, size_t node_count, Linkage* linkage,
                       InstructionSequence* sequence, Schedule* schedule,
                       SourcePositionTable* source_positions,
                       Features features = SupportedFeatures());
@@ -126,9 +121,7 @@ class InstructionSelector FINAL {
   bool IsLive(Node* node) const { return !IsDefined(node) && IsUsed(node); }
 
   int GetVirtualRegister(const Node* node);
-  // Gets the current mapping if it exists, kNodeUnmapped otherwise.
-  int GetMappedVirtualRegister(const Node* node) const;
-  const NodeToVregMap& GetNodeMapForTesting() const { return node_map_; }
+  const std::map<NodeId, int> GetVirtualRegistersForTesting() const;
 
  private:
   friend class OperandGenerator;
@@ -222,11 +215,11 @@ class InstructionSelector FINAL {
   SourcePositionTable* const source_positions_;
   Features features_;
   Schedule* const schedule_;
-  NodeToVregMap node_map_;
   BasicBlock* current_block_;
-  ZoneDeque<Instruction*> instructions_;
+  ZoneVector<Instruction*> instructions_;
   BoolVector defined_;
   BoolVector used_;
+  IntVector virtual_registers_;
 };
 
 }  // namespace compiler

src/compiler/pipeline.cc

@@ -531,7 +531,7 @@ struct InstructionSelectionPhase {
   static const char* phase_name() { return "select instructions"; }
 
   void Run(PipelineData* data, Zone* temp_zone, Linkage* linkage) {
-    InstructionSelector selector(temp_zone, data->graph(), linkage,
+    InstructionSelector selector(temp_zone, data->graph()->NodeCount(), linkage,
                                  data->sequence(), data->schedule(),
                                  data->source_positions());
     selector.SelectInstructions();

src/zone-containers.h

@@ -27,12 +27,12 @@ class ZoneVector : public std::vector<T, zone_allocator<T>> {
 
   // Constructs a new vector and fills it with {size} elements, each
   // constructed via the default constructor.
-  ZoneVector(int size, Zone* zone)
+  ZoneVector(size_t size, Zone* zone)
       : std::vector<T, zone_allocator<T>>(size, T(), zone_allocator<T>(zone)) {}
 
   // Constructs a new vector and fills it with {size} elements, each
   // having the value {def}.
-  ZoneVector(int size, T def, Zone* zone)
+  ZoneVector(size_t size, T def, Zone* zone)
       : std::vector<T, zone_allocator<T>>(size, def, zone_allocator<T>(zone)) {}
 };
 

test/unittests/compiler/instruction-selector-unittest.cc

@@ -34,14 +34,14 @@ InstructionSelectorTest::Stream InstructionSelectorTest::StreamBuilder::Build(
     out << "=== Schedule before instruction selection ===" << std::endl
         << *schedule;
   }
-  EXPECT_NE(0, graph()->NodeCount());
-  int initial_node_count = graph()->NodeCount();
+  size_t const node_count = graph()->NodeCount();
+  EXPECT_NE(0u, node_count);
   Linkage linkage(test_->zone(), call_descriptor());
   InstructionBlocks* instruction_blocks =
       InstructionSequence::InstructionBlocksFor(test_->zone(), schedule);
   InstructionSequence sequence(test_->zone(), instruction_blocks);
   SourcePositionTable source_position_table(graph());
-  InstructionSelector selector(test_->zone(), graph(), &linkage, &sequence,
+  InstructionSelector selector(test_->zone(), node_count, &linkage, &sequence,
                                schedule, &source_position_table, features);
   selector.SelectInstructions();
   if (FLAG_trace_turbo) {
@@ -52,19 +52,9 @@ InstructionSelectorTest::Stream InstructionSelectorTest::StreamBuilder::Build(
         << printable;
   }
   Stream s;
+  s.virtual_registers_ = selector.GetVirtualRegistersForTesting();
   // Map virtual registers.
-  {
-    const NodeToVregMap& node_map = selector.GetNodeMapForTesting();
-    for (int i = 0; i < initial_node_count; ++i) {
-      if (node_map[i] != InstructionSelector::kNodeUnmapped) {
-        s.virtual_registers_.insert(std::make_pair(i, node_map[i]));
-      }
-    }
-  }
-  std::set<int> virtual_registers;
-  for (InstructionSequence::const_iterator i = sequence.begin();
-       i != sequence.end(); ++i) {
-    Instruction* instr = *i;
+  for (Instruction* const instr : sequence) {
     if (instr->opcode() < 0) continue;
     if (mode == kTargetInstructions) {
       switch (instr->arch_opcode()) {
@@ -86,10 +76,6 @@ InstructionSelectorTest::Stream InstructionSelectorTest::StreamBuilder::Build(
       if (output->IsConstant()) {
         s.constants_.insert(std::make_pair(
             output->index(), sequence.GetConstant(output->index())));
-        virtual_registers.insert(output->index());
-      } else if (output->IsUnallocated()) {
-        virtual_registers.insert(
-            UnallocatedOperand::cast(output)->virtual_register());
       }
     }
     for (size_t i = 0; i < instr->InputCount(); ++i) {
@@ -98,16 +84,12 @@ InstructionSelectorTest::Stream InstructionSelectorTest::StreamBuilder::Build(
       if (input->IsImmediate()) {
         s.immediates_.insert(std::make_pair(
             input->index(), sequence.GetImmediate(input->index())));
-      } else if (input->IsUnallocated()) {
-        virtual_registers.insert(
-            UnallocatedOperand::cast(input)->virtual_register());
       }
     }
     s.instructions_.push_back(instr);
   }
-  for (std::set<int>::const_iterator i = virtual_registers.begin();
-       i != virtual_registers.end(); ++i) {
-    int virtual_register = *i;
+  for (auto i : s.virtual_registers_) {
+    int const virtual_register = i.second;
     if (sequence.IsDouble(virtual_register)) {
       EXPECT_FALSE(sequence.IsReference(virtual_register));
       s.doubles_.insert(virtual_register);