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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay requested changes to this revision.
MaskRay added inline comments.


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:31
+#include "llvm/MC/MCTargetOptionsCommandFlags.h"
+#include "llvm/MC/SubtargetFeature.h"
+#include "llvm/MC/TargetRegistry.h"
----------------
and clang-format this file.


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:282
+      unwrapOrError(object::createBinary(InputFilename));
+
+  object::Binary &Binary = *BinaryOrErr.getBinary();
----------------
We usually use a compact style. A variable declaration doesn't need a following blank line.

BinaryOrErr and Binary are quite related. Adding a blank line only harms readability.


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:290
+  std::string Error;
+
+  const Target *TheTarget = TargetRegistry::lookupTarget(TripleName, Error);
----------------
jhenderson wrote:
> There are a lot of unnecessary blank lines within this function, which disrupt the flow of someone reading it. Keep your code grouped into logical bits, e.g. variable declarations and the function that then uses them all together.
> 
> Also don't initialise local variables until you need them. E.g. `TheTarget` isn't used until quite a way down from here, so move its initialization until then.
delete blank line after `std::string Error`


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:300
+  SubtargetFeatures TrueFeatures = *Features;
+
+  
----------------
We almost never use 2 blank lines for logical separation.

In this case, `TrueFeatures` is immediately used and should have no blank lines following it.

getFeatures only returns a non-empty string. You need an AArch32/RISC-V/Mips test to test it.


================
Comment at: llvm/tools/llvm-cm/llvm-cm.cpp:303
+  for (unsigned I = 0; I != MAttrs.size(); ++I) {
+      TrueFeatures.AddFeature(MAttrs[I]);
+  }
----------------
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`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153376