[PATCH] D127083: [MCA] Introducing incremental SourceMgr and resumable pipeline

[Min-Yih Hsu via Phabricator via llvm-commits]

myhsu added inline comments.


================
Comment at: llvm/unittests/tools/llvm-mca/CMakeLists.txt:26
+
+function(add_llvm_mca_unittest_includes)
+  set(mca_includes ${mca_includes} ${ARGV} PARENT_SCOPE)
----------------
thakis wrote:
> Why is this file so complicated? All other CMakeLists.txt in llvm/unittests are very simple.
> 
> It kind of looks like this links in some, but not all, code in tools/llvm-mca/Views in to the test, instead of doing a library-level dependency. That seems
> a) strange from a layering PoV
> b) ODR-violation-prone in the future
> 
> Why is this designed the way it is, instead of more like all the other tests?
This file is taking references from unittest/tools/llvm-exegesis/CMakeLists.txt (which is actually more complicated than this one) because llvm-mca has a similar need with llvm-exegesis.

The rationale behind this is that we want to build in-memory MCInst objects as the test input (which requires target specific header files) for a simpler test harness.
That said, I think moving files under tools/llvm-mca/Views into include/llvm/MCA/Views (and its lib/MCA counterpart) is a good future idea @andreadb what do you think?


================
Comment at: llvm/unittests/tools/llvm-mca/MCATestBase.cpp:85
+        // Default case.
+        return std::move(NewE);
+      }
----------------
hubert.reinterpretcast wrote:
> This is causing failures:
> https://lab.llvm.org/buildbot/#/builders/57/builds/19267
> 
> ```
> /home/buildbots/docker-RHEL-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/llvm/unittests/tools/llvm-mca/MCATestBase.cpp:85:16: error: moving a local object in a return statement prevents copy elision [-Werror,-Wpessimizing-move]
>         return std::move(NewE);
>                ^
> /home/buildbots/docker-RHEL-buildbot/SetupBot/worker_env/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/llvm/unittests/tools/llvm-mca/MCATestBase.cpp:85:16: note: remove std::move call here
>         return std::move(NewE);
>                ^~~~~~~~~~    ~
> 1 error generated.
> ```
Sorry for that, will be fixed shortly...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127083