[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/CMakeLists.txt:20
+#set(LLVM_CM_SOURCE_DIR ${CURRENT_SOURCE_DIR})
\ No newline at end of file

----------------
needs a newline here (makes diff happy)


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:55
+
+static uint64_t StartAddr; 
+static uint64_t StopAddr = UINT64_MAX; 
----------------
why do these need to be static, can it be scoped elsewhere?

also, please initialize at declaration (easier to avoid use before init)


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:56
+static uint64_t StartAddr; 
+static uint64_t StopAddr = UINT64_MAX; 
+std::vector<std::string> FilterSections; 
----------------
is this a `const`?


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:57
+static uint64_t StopAddr = UINT64_MAX; 
+std::vector<std::string> FilterSections; 
+StringSet<> FoundSectionSet; 
----------------
`FilterSections` isn't used, so probably something for a future patch. Until then, it should be removed.


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:76
+  // True if the section should not be skipped.
+  bool Keep;
+
----------------
init at declaration


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:185
+
+    /*
+    // TEMP CHECK: Get the name of the object file and its format and print it 
----------------
please remove commented code.


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:200
+    if (!TheTarget) { 
+        errs() << argv[0] << ": " << Error; 
+        return 1; 
----------------
why not use the `error` function above?


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:223
+    if (!MRI) {
+        WithColor::error() << "error: no register info for target " << TripleName
+                     << "\n";
----------------
why not use `error`? (same further below)

should `error()` do `WithColor` instead - basically, it's not clear why there are more than one way to report errors.


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:315
+    for (const object::SectionRef &Section : toolSectionFilter(*Obj, nullptr)) { 
+      if (FilterSections.empty() && (!Section.isText() || Section.isVirtual())) {
+        continue;  
----------------
(coding style) you don't need `{` `}` for single-line blocks


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:320
+
+      uint64_t SectionAddr = Section.getAddress(); 
+      uint64_t SectionSize = Section.getSize(); 
----------------
you could `const` these (readability)


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:323
+
+      if (!SectionSize) {
+        continue; 
----------------
single block


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:323
+
+      if (!SectionSize) {
+        continue; 
----------------
mtrofin wrote:
> single block
you can probably do some numerical validation here, like an assert or test that "SectionAddr <= maxuint64 - SectionSize"


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:340
+
+      uint64_t Size; 
+
----------------
init at decl.


also this seems to be used further down in the loop ~line 388, so please move it there.


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:344
+      for (size_t SI = 0, SE = Symbols.size(); SI != SE;) {
+        unsigned FirstSI = SI;
+        uint64_t Start = Symbols[SI].Addr;
----------------
why narrow the representation, keep it `size_t`? also `const` it?


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:357
+
+        uint64_t End = std::min<uint64_t>(SectionAddr + SectionSize, StopAddr);
+        if (SI < SE)
----------------
StopAddr is max uint64, so don't quite follow here. 


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:360
+          End = std::min(End, Symbols[SI].Addr);
+        if (Start >= End || End <= StartAddr)
+          continue;
----------------
Are the valid values Start-inclusive and End-exclusive, i.e. should one of these be strict inequality?


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:373
+
+        for (size_t I = 0; I < SymbolsHere.size(); I++) { 
+        
----------------
++I


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


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:393
+                                                 CommentStream); 
+          //Inst.dump(); 
+          NumInstructions++; 
----------------
remove commented code.


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:394
+          //Inst.dump(); 
+          NumInstructions++; 
+          if (Size == 0) {
----------------
++NumInstructions


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:399
+
+          if (!Disassembled) {
+            WithColor::warning() << "invalid instruction encoding\n"; 
----------------
can you do

if (!DisAsm->getInstruction...) {
  WithColor...
  break;
}


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:410
+}
\ No newline at end of file

----------------
newline


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153376