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

Fangrui Song via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jun 29 20:25:59 PDT 2023

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`

  rG LLVM Github Monorepo



More information about the llvm-commits mailing list