[PATCH] D153376: Introducing llvm-cm: A Cost Model Tool

[Dayann D'almeida via Phabricator via llvm-commits]

JestrTulip added inline comments.


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:74
+// Rewrite ExitIf to override the << operator to print the message. 
+class ExitIf {
+   public:
----------------
jhenderson wrote:
> Why is this a class when a simple function would do?
Going off @ondrasej's suggestion regarding RAII. 


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:164
+
+// TODO: Share this with llvm-objdump.cpp. 
+static uint8_t getElfSymbolType(const llvm::object::ObjectFile &Obj, const llvm::object::SymbolRef &Sym) {
----------------
jhenderson wrote:
> Any reason you can't do this upfront, like you did with the section filter stuff?
I was planning on including these changes in a later patch, since the overall scope of this one is so minimal, I wanted to avoid changing many other files. 


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:303
+  for (unsigned I = 0; I != MAttrs.size(); ++I) {
+      TrueFeatures.AddFeature(MAttrs[I]);
+  }
----------------
MaskRay wrote:
> 2-space indentation. omit braces for a single-line simple statement.
> 
> Features may start with `-` indicating a negative feature. It's incorrect to use `TrueFeatures`
Changed to remove TrueFeatures and MAttrs


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:188
+// TODO: Share this with llvm-objdump.cpp. 
+static void collectBBtoAddressLabels(const std::unordered_map<uint64_t, llvm::object::BBAddrMap> &AddrToBBAddrMap,
+                       uint64_t SectionAddr, uint64_t Start, uint64_t End,
----------------
mtrofin wrote:
> this is from subsequent patch?
it's for the current one, it maps the addresses we use for the MCInsts with the labels from the basic blocks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153376