[PATCH] D54185: [PowerPC][llvm-exegesis] Add a PowerPC target

[Jinsong Ji via Phabricator via llvm-commits]

jsji added a comment.

Thanks for prompt review, I will update the patch to fix the issues.



================
Comment at: llvm/tools/llvm-exegesis/lib/CMakeLists.txt:13
+  add_subdirectory(PowerPC)
+  set(LLVM_EXEGESIS_TARGETS "${LLVM_EXEGESIS_TARGETS} PowerPC" PARENT_SCOPE)
+endif()
----------------
courbet wrote:
> I think you wrote this against the version before PR39021 and failed to merge the changes in r342865.
Yes, you are right! I did a quick update before submitting the patch, and apparently I didn't merge all changes correctly. I will have a look at r342865, and update the patch. Thanks.


================
Comment at: llvm/tools/llvm-exegesis/lib/PowerPC/Target.cpp:8
+//
+// Register PowerPC target with default latency benchmarks runner
+//===----------------------------------------------------------------------===//
----------------
courbet wrote:
> "The PowerPC ExegesisTarget."
OK


================
Comment at: llvm/tools/llvm-exegesis/lib/PowerPC/Target.cpp:32
+
+namespace {
+static unsigned GetLoadImmediateOpcode(unsigned RegBitWidth) {
----------------
courbet wrote:
> remove the namespace; LLVM style does not put static functions in anonymous namespaces.
OK


================
Comment at: llvm/tools/llvm-exegesis/lib/PowerPC/Target.cpp:33
+namespace {
+static unsigned GetLoadImmediateOpcode(unsigned RegBitWidth) {
+  switch (RegBitWidth) {
----------------
courbet wrote:
> `getLoadImmediateOpcode`
OK


================
Comment at: llvm/unittests/tools/llvm-exegesis/PowerPC/AnalysisTest.cpp:1
+#include "Analysis.h"
+
----------------
courbet wrote:
> This file is missing a header.
I'll have a look, Thanks.


Repository:
  rL LLVM

https://reviews.llvm.org/D54185