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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-cm/X86/bad_triple.s:2
+# RUN: llvm-mc -o %t.o --filetype=obj -triple=x86_64-unknown-linux-gnu %s 
+# RUN: not llvm-cm -triple=not_real_triple %t.o 2>&1 | FileCheck %s	
+
----------------
Do you actually need a valid object for this test? I would expect the check for a valid triple to occur before handling of the input file. If you do need a valid object, that's fine, but can it just be a trivial file, i.e. one made from an empty/single-line asm file?


================
Comment at: llvm/test/tools/llvm-cm/X86/bad_triple.s:34
+
+# CHECK: llvm-cm: error: No available targets are compatible with triple "not_real_triple" 
----------------
It's easier to follow the test if this check appears before the large block of asm. Same generally goes throughout.


================
Comment at: llvm/test/tools/llvm-cm/X86/empty.s:1
+## Check that llvm-cm does not crash on an empty input file.
+# RUN: llvm-mc -o %t.o --filetype=obj -triple=x86_64-unknown-linux-gnu %s 
----------------
Let's be more specific: "Check that llvm-cm produces no output for an empty input file."


================
Comment at: llvm/test/tools/llvm-cm/X86/malformed.s:1
+## Check that llvm-cm returns an error when run on a non-object file 
+# RUN: not llvm-cm %s 2>&1 | FileCheck %s 
----------------
Please be careful of trailing whitespace too. I see it in other test files.


================
Comment at: llvm/test/tools/llvm-cm/X86/multi_funct.s:1
+# RUN: llvm-mc -o %t.o --filetype=obj -triple=x86_64-unknown-linux-gnu %s 
+# RUN: llvm-cm %t.o 2>&1 | FileCheck %s
----------------
I think a comment explaining each test would be a good idea.

Also "func" is a more common abbreviation for "function" than "funct".

Also, prefer `-` in test names over `_` due to it being easier to type.


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:289
+  std::unique_ptr<MCRegisterInfo> MRI(TheTarget->createMCRegInfo(TripleName));
+  ExitIf(!MRI) << "no register info for target " + TripleName + "\n";
+
----------------
Your ExitIf class writes a '\n' after the messsge, so there's no need for an additional one here and elsewhere you've added it.


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:325
+  bool Is64Bits = Obj->getBytesInAddress() > 4;
+
+  for (const object::SymbolRef &Symbol : Obj->symbols()) {
----------------
As above, delete this blank line, so that the declarations tied to the loop are tightly linked to it.


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:117
+    WithColor::warning(errs(), "llvm-cm")
+        << "Failed to get section name: " << toString(SecNameOrErr.takeError())
+        << "\n";
----------------
JestrTulip wrote:
> jhenderson wrote:
> > https://llvm.org/docs/CodingStandards.html#error-and-warning-messages
> > 
> > Also, test case?
> Regarding the test cases for aspects such as Section Name, a good portion of the error handling for disassembly was modeled off the error handling in tools such as objdump and mca. For this specific case, and many of those that are unaddressed within the program, I find it hard to come up with a way to properly test these occurrences. 
> For example, this error requires a Section to exist within the file, but its name to not be found. 
> If I could receive any ideas on how to properly test situations like this, i'd be very grateful. 
> 
> 
Take a look at the test cases for llvm-readobj - there are many examples where it uses yaml2obj to customise the input in some way, e.g. creating an input without a section header string table (see https://github.com/llvm/llvm-project/blob/main/llvm/test/tools/llvm-readobj/ELF/sections-no-section-header-string-table.test). You should be able to use yaml2obj to achieve a broken input in some manner using these examples (more can be found in the ELF yaml2obj tests too, if you want to explore the various options you have). NB: you don't need to test every possible failure mode that the underlying library can hit (these should already have testing elsewhere). Instead, you should make sure you have testing that covers the case where a particularly library function returns an error, to show that you handled the error correctly.


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:75
+class ExitIf {
+   public:
+    ExitIf(bool Cond) : Condition(Cond) {}
----------------
JestrTulip wrote:
> jhenderson wrote:
> > jhenderson wrote:
> > > Have you run clang-format on your new code? Because this doesn't look formatted according to the standard format.
> > Ping - not addressed.
> I've run git clang-format on the most recent patch.  Just to make sure I'm doing it correctly: 
> I first git added all the modified files, 
> I then ran git-clang format from the root of my repo 
> I then received the message "clang-format did not modify any files" 
>  
I personally don't use git-clang-format to do it - I use a clang-format tool that comes along with my Visual Studio setup, so I can't comment. Perhaps @MaskRay or another review could assist, because this is definitely not correctly formatted.


================
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) {
----------------
JestrTulip wrote:
> 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. 
This is brand new code. You should really avoid duplicating code in new code, if at all possible. In other words, you should be refactoring the code you want to use in earlier patches, to make it shareable, and then base this patch on top of them.


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:70
+// Class for error handling.  
+class ExitIf {
+   public:
----------------
JestrTulip wrote:
> jhenderson wrote:
> > ondrasej wrote:
> > > mtrofin wrote:
> > > > nit: `class ExitIf final`
> > > > 
> > > > upon looking more closely at the usage pattern - @ondrasej , why raii and not just a function call? raii will exit at scope exit, which is undesirable (the goal is to exit right away). A function call "just works" because `exit(1)`, no?
> > > As discussed offline yesterday: RAII is there to make the streaming operators after `ExitIf()` work.
> > > 
> > > With RAII, and with the intended use (`ExitIf(Cond) << "Some message";`), the following happens:
> > > 1. a temporary `ExitIf` instance is created and takes note of `Cond`,
> > > 2. all streaming operators are applied to the temporary (collecting the messages),
> > > 3. it's a temporary value (there's no variable name), so its scope is limited to the statement where it appears.
> > > 
> > > Basically, it processes the chain of `<<`, and then immediately exits.
> > Why do you need RAII for this? You could just do string concatenation to get your final message, and pass that into a function that prints the message and then calls `std::exit`.
> The reason for not doing string concatenation to get the message and passing the result into a function is mainly due of the reduced overhead (specifically, the temporary string objects) the class option provides.  With string concatenation, in all cases, we must perform extra object allocations to get the final message. 
> However, with this implementation, we only assemble the message into the stringstream when there's an error. 
> I realize this was not exactly what original implementation did,  but I've updated it alongside my most recent update. If this implementation is not preferred, I am perfectly able to accommodate.
LLVM has the `Twine` class to support efficient string concatenation, which would defer the real concatenation until needed (see https://llvm.org/docs/ProgrammersManual.html#the-twine-class). By passing the concatenated `Twine` to the error function for printing if the error is hit, you'll avoid any unnecessary string processing cost in the non-failing case, whilst keeping the interface simple. As a bonus, `Twine` takes constructors that do `std::to_string`-like conversions of its input, so that you can get the same functionality as the streaming option.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153376