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

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added inline comments.


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:122
+      },
+      O);
+}
----------------
nit: can you put /*param_name=*/ for the `0` here, too?


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:140
+
+static uint8_t getElfSymbolType(const llvm::object::ObjectFile &Obj, const llvm::object::SymbolRef &Sym) {
+  assert(Obj.isELF());
----------------
nit: can you add a TODO here if this is something that could be shared with llvm-objdump - so we don't forget.


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:169
+void printFunction(ArrayRef<SymbolInfoTy> &Aliases,
+               std::vector<StringRef> &CurrSymName) {
+  for (size_t I = 0; I < Aliases.size(); ++I) {
----------------
const std::vector<StringRef>&


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:205
+
+  EXIT_IF(!Features, errs() << "error: "
+                            << "Features not defined"
----------------
did you mean to put the error after error:?


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:211
+
+  if (MAttrs.empty()) {
+    for (unsigned I = 0; I != MAttrs.size(); ++I) {
----------------
when won't it be empty?


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:254
+
+  int AsmPrinterVariant = AsmInfo->getAssemblerDialect();
+
----------------
you can avoid having this variable around by just creating IP with `AsmInfo->getAssemblerDialect()`. Or const it.


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:256
+
+  // Create the MCInstPrinter (just in case)
+  std::unique_ptr<MCInstPrinter> IP(TheTarget->createMCInstPrinter(
----------------
I don't follow: in case what? :)


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:365
+
+      printFunction(Aliases, CurrSymName);
+
----------------
why do you need to pass Aliases here?




================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:76
+  // True if the section should not be skipped.
+  bool Keep;
+
----------------
mtrofin wrote:
> init at declaration
`Keep` and `IncrementIndex` can be init-ed at declaration (probably the anchor of this comment moved, but it's convenient :) )


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:373
+
+        for (size_t I = 0; I < SymbolsHere.size(); I++) { 
+        
----------------
mtrofin wrote:
> mtrofin wrote:
> > ++I
> you can probably factor this out in a function
it's still not factored


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153376