[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:
> myhsu wrote:
> > 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?
> For example, the llvm/unittests/tools/llvm-exegesis/* does something somewhat similar (…I think?) with much less machinery, by having a test binary per target. Could that design work here?
> For example, the llvm/unittests/tools/llvm-exegesis/* does something somewhat similar (…I think?) with much less machinery, by having a test binary per target. Could that design work here?

I don't think so, there is only a single test binary, LLVMExegesisTests, same as us. We also assigned each target a folder (currently there is only X86).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127083