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

[Ondrej Sykora via Phabricator via llvm-commits]

ondrasej added inline comments.


================
Comment at: llvm/test/tools/llvm-cm/inst_count.ll:15-16
+
+; CHECK: # of instructions: 4
+; CHECK: # of instructions: 3 
+
----------------
This might be a bit brittle - the best option would be to use assembly directly, if possible.


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:71
+    exit(1);                                                                   \
+  } while (false)
+
----------------
I'd prefer to avoid using this type of macros:
- they tend to break automatic code manipulation tools,
- get brittle around code that uses commas,
- they are not commonly used in the LLVM code base.

If you really want to use this pattern, I'd go with something like
```
class ExitIf {
   public:
    ExitIf(bool Cond) : Condition(Cond) {}
    ~ExitIf() {
        if (Condition) {
            std::cerr << MsgStream.str() << std::endl;
            exit(1);
        }
    }

    template <typename T>
    ExitIf& operator<<(const T& other) {
        MsgStream << other;
        return *this;
    }

   private:
    bool Condition;
    std::stringstream MsgStream;  // Or llvm::raw_ostream.
};

// Used as:
ExitIf(!Foo) << "Foo is not true :(";
```

It will format the message even if condition holds, but that's probably OK in this case (and can be fixed with a relatively simple macro).

There's also `llvm::ExitOnError` that might help in a few cases here.


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:125
+
+// Implement the "error" function
+[[noreturn]] static void error(Error Err) {
----------------
Please add a period at the end of the comment.

Same for all the other comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153376