[PATCH] D86974: [IRSim] Adding basic implementation of llvm-sim.

[Andrew Litteken via Phabricator via llvm-commits]

AndrewLitteken added inline comments.


================
Comment at: llvm/tools/llvm-sim/llvm-sim.cpp:133
+    for (BasicBlock &BB : F)
+      for (Instruction &I : BB)
+        LLVMInstNum.insert(std::make_pair(&I, InstructionNumber++));
----------------
vsk wrote:
> If it's not necessary/desirable to number meta-instructions (e.g. debug info intrinsics), `BB.instructionsWithoutDebug()` might be a better fit here.
I agree, that would probably be better here.


================
Comment at: llvm/tools/llvm-sim/llvm-sim.cpp:134
+      for (Instruction &I : BB)
+        LLVMInstNum.insert(std::make_pair(&I, InstructionNumber++));
+
----------------
vsk wrote:
> Very minor nit -- any reason not to prefer a shorter way to write this, maybe 'map[&I] = Num++' or 'map.emplace(&I, Num++)' if it's necessary to avoid default-constructing a DenseMapPair?
I don't think so. I just didn't realize those shortcuts existed.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86974/new/

https://reviews.llvm.org/D86974