From 30d2d67bb0e8134a80c36f0aa145e914955af7f6 Mon Sep 17 00:00:00 2001 From: MikePopoloski Date: Sun, 4 Feb 2024 14:32:17 -0500 Subject: [PATCH] Add new -C option to specify a separately-compiled unit file listing --- bindings/python/CompBindings.cpp | 5 +- include/slang/driver/Driver.h | 6 +- include/slang/driver/SourceLoader.h | 37 ++++++++++-- source/driver/Driver.cpp | 91 +++++++++++++++++++++++++---- source/driver/SourceLoader.cpp | 83 ++++++++++++++++++++++++-- tests/unittests/DriverTests.cpp | 31 ++++++++++ tests/unittests/data/test5.sv | 2 +- tests/unittests/data/unit.f | 3 + 8 files changed, 235 insertions(+), 23 deletions(-) create mode 100644 tests/unittests/data/unit.f diff --git a/bindings/python/CompBindings.cpp b/bindings/python/CompBindings.cpp index 32a29d98b..1d0bbe648 100644 --- a/bindings/python/CompBindings.cpp +++ b/bindings/python/CompBindings.cpp @@ -170,7 +170,8 @@ void registerCompilation(py::module_& m) { return self.parseCommandLine(arg, parseOptions); }, "arg"_a, "parseOptions"_a = CommandLine::ParseOptions{}) - .def("processCommandFiles", &Driver::processCommandFiles, "fileName"_a, "makeRelative"_a) + .def("processCommandFiles", &Driver::processCommandFiles, "fileName"_a, "makeRelative"_a, + "separateUnit"_a) .def("processOptions", &Driver::processOptions) .def("runPreprocessor", &Driver::runPreprocessor, "includeComments"_a, "includeDirectives"_a, "obfuscateIds"_a, "useFixedObfuscationSeed"_a = false) @@ -196,6 +197,8 @@ void registerCompilation(py::module_& m) { .def("addSearchExtension", &SourceLoader::addSearchExtension, "extension"_a) .def("addLibraryMaps", &SourceLoader::addLibraryMaps, "pattern"_a, "basePath"_a, "optionBag"_a) + .def("addSeparateUnit", &SourceLoader::addSeparateUnit, "filePatterns"_a, "includePaths"_a, + "defines"_a, "libraryName"_a) .def("loadSources", &SourceLoader::loadSources) .def("loadAndParseSources", &SourceLoader::loadAndParseSources, "optionBag"_a) .def_property_readonly("hasFiles", &SourceLoader::hasFiles) diff --git a/include/slang/driver/Driver.h b/include/slang/driver/Driver.h index f9fce77f2..256e86dfe 100644 --- a/include/slang/driver/Driver.h +++ b/include/slang/driver/Driver.h @@ -250,8 +250,11 @@ class SLANG_EXPORT Driver { /// @param pattern a file path pattern indicating the command file(s) to process. /// @param makeRelative indicates whether paths in the file are relative to the file /// itself or to the current working directory. + /// @param separateUnit if true, the file is a separate compilation unit listing; + /// options within it apply only to that unit and not the + /// broader compilation. /// @returns true on success and false if errors were encountered. - bool processCommandFiles(std::string_view pattern, bool makeRelative); + bool processCommandFiles(std::string_view pattern, bool makeRelative, bool separateUnit); /// Processes and applies all configured options. /// @returns true on success and false if errors were encountered. @@ -297,6 +300,7 @@ class SLANG_EXPORT Driver { [[nodiscard]] bool reportCompilation(ast::Compilation& compilation, bool quiet); private: + bool parseUnitListing(std::string_view text); void addLibraryFiles(std::string_view pattern); void addParseOptions(Bag& bag) const; void addCompilationOptions(Bag& bag) const; diff --git a/include/slang/driver/SourceLoader.h b/include/slang/driver/SourceLoader.h index b148dbf56..ee8b117d4 100644 --- a/include/slang/driver/SourceLoader.h +++ b/include/slang/driver/SourceLoader.h @@ -7,6 +7,7 @@ //------------------------------------------------------------------------------ #pragma once +#include #include #include #include @@ -110,6 +111,19 @@ class SLANG_EXPORT SourceLoader { void addLibraryMaps(std::string_view pattern, const std::filesystem::path& basePath, const Bag& optionBag); + /// @brief Adds a group of files as a separately compiled compilation unit. + /// + /// Unlike files added via the @a addFiles method, files added here are + /// all guaranteed to be grouped into a single compilation unit and use + /// the provided options for preprocessor defines and include paths. + /// + /// If the library name is provided the compilation unit will be included + /// in the library of that name; otherwise it will be included in the + /// default library and be considered a non-library unit. + void addSeparateUnit(std::span filePatterns, + std::vector includePaths, std::vector defines, + std::string libraryName); + /// Returns a list of all library map syntax trees that have been loaded and parsed. const SyntaxTreeList& getLibraryMaps() const { return libraryMapTrees; } @@ -128,6 +142,14 @@ class SLANG_EXPORT SourceLoader { std::span getErrors() const { return errors; } private: + // One entry per unit of files + options to compile them. + // Only used for addSeparateUnit. + struct UnitEntry { + std::vector includePaths; + std::vector defines; + const SourceLibrary* library = nullptr; + }; + // One entry per unique file path added to the loader. struct FileEntry { // The filesystem path (as specified by the user). @@ -144,6 +166,9 @@ class SLANG_EXPORT SourceLoader { // matches at an even higher rank. const SourceLibrary* secondLib = nullptr; + // A pointer to the unit this file is a part of, or nullptr if none. + const UnitEntry* unit = nullptr; + // A measure of how strongly this file belongs to the library. GlobRank libraryRank; @@ -156,22 +181,25 @@ class SLANG_EXPORT SourceLoader { bool isLibraryFile = false; FileEntry(std::filesystem::path&& path, bool isLibraryFile, const SourceLibrary* library, - GlobRank libraryRank) : + const UnitEntry* unit, GlobRank libraryRank) : path(std::move(path)), - library(library), libraryRank(libraryRank), isLibraryFile(isLibraryFile) {} + library(library), unit(unit), libraryRank(libraryRank), isLibraryFile(isLibraryFile) {} }; // The result of a loadAndParse call. // 0: A parsed syntax tree // 1: A loaded source buffer + bool that indicates whether it's a library // 2: A file entry + error code if the load fails + // 3: A source buffer + unit pointer if it's part of a separate unit using LoadResult = std::variant, std::pair, - std::pair>; + std::pair, + std::pair>; SourceLibrary* getOrAddLibrary(std::string_view name); void addFilesInternal(std::string_view pattern, const std::filesystem::path& basePath, - bool isLibraryFile, const SourceLibrary* library, bool expandEnvVars); + bool isLibraryFile, const SourceLibrary* library, const UnitEntry* unit, + bool expandEnvVars); void addLibraryMapsInternal(std::string_view pattern, const std::filesystem::path& basePath, const Bag& optionBag, bool expandEnvVars, flat_hash_set& seenMaps); @@ -186,6 +214,7 @@ class SLANG_EXPORT SourceLoader { std::vector fileEntries; flat_hash_map fileIndex; flat_hash_map> libraries; + std::deque unitEntries; std::vector searchDirectories; std::vector searchExtensions; flat_hash_set uniqueExtensions; diff --git a/source/driver/Driver.cpp b/source/driver/Driver.cpp index e3cf20a00..fc3c53c56 100644 --- a/source/driver/Driver.cpp +++ b/source/driver/Driver.cpp @@ -105,6 +105,17 @@ void Driver::addStandardArgs() { cmdLine.add("-j,--threads", options.numThreads, "The number of threads to use to parallelize parsing", ""); + cmdLine.add( + "-C", + [this](std::string_view value) { + processCommandFiles(value, /* makeRelative */ true, /* separateUnit */ true); + return ""; + }, + "One or more files containing independent compilation unit listings. " + "The files accept a subset of options that pertain specifically to parsing " + "that unit and optionally including it in a library.", + "[,...]", CommandLineFlags::CommaList); + // Compilation cmdLine.add("--max-hierarchy-depth", options.maxInstanceDepth, "Maximum depth of the design hierarchy", ""); @@ -289,7 +300,7 @@ void Driver::addStandardArgs() { cmdLine.add( "-f", [this](std::string_view value) { - processCommandFiles(value, /* makeRelative */ false); + processCommandFiles(value, /* makeRelative */ false, /* separateUnit */ false); return ""; }, "One or more command files containing additional program options. " @@ -299,7 +310,7 @@ void Driver::addStandardArgs() { cmdLine.add( "-F", [this](std::string_view value) { - processCommandFiles(value, /* makeRelative */ true); + processCommandFiles(value, /* makeRelative */ true, /* separateUnit */ false); return ""; }, "One or more command files containing additional program options. " @@ -317,7 +328,7 @@ void Driver::addStandardArgs() { return !anyFailedLoads; } -bool Driver::processCommandFiles(std::string_view pattern, bool makeRelative) { +bool Driver::processCommandFiles(std::string_view pattern, bool makeRelative, bool separateUnit) { auto onError = [this](const auto& name, std::error_code ec) { printError(fmt::format("command file '{}': {}", name, ec.message())); anyFailedLoads = true; @@ -349,17 +360,22 @@ bool Driver::processCommandFiles(std::string_view pattern, bool makeRelative) { fs::current_path(path.parent_path(), ec); } - CommandLine::ParseOptions parseOpts; - parseOpts.expandEnvVars = true; - parseOpts.ignoreProgramName = true; - parseOpts.supportComments = true; - parseOpts.ignoreDuplicates = true; - SLANG_ASSERT(!buffer.empty()); buffer.pop_back(); - std::string_view argStr(buffer.data(), buffer.size()); - const bool result = parseCommandLine(argStr, parseOpts); + + bool result; + if (separateUnit) { + result = parseUnitListing(argStr); + } + else { + CommandLine::ParseOptions parseOpts; + parseOpts.expandEnvVars = true; + parseOpts.ignoreProgramName = true; + parseOpts.supportComments = true; + parseOpts.ignoreDuplicates = true; + result = parseCommandLine(argStr, parseOpts); + } if (makeRelative) fs::current_path(currPath, ec); @@ -775,6 +791,59 @@ bool Driver::reportCompilation(Compilation& compilation, bool quiet) { return succeeded; } +bool Driver::parseUnitListing(std::string_view text) { + CommandLine unitCmdLine; + std::vector includes; + unitCmdLine.add("-I,--include-directory,+incdir", includes, "", "", + CommandLineFlags::CommaList); + + std::vector defines; + unitCmdLine.add("-D,--define-macro,+define", defines, ""); + + std::optional libraryName; + unitCmdLine.add("--library", libraryName, ""); + + unitCmdLine.add( + "-C", + [this](std::string_view value) { + processCommandFiles(value, /* makeRelative */ true, /* separateUnit */ true); + return ""; + }, + "", "", CommandLineFlags::CommaList); + + std::vector files; + unitCmdLine.setPositional( + [&](std::string_view value) { + if (!options.excludeExts.empty()) { + if (size_t extIndex = value.find_last_of('.'); extIndex != std::string_view::npos) { + if (options.excludeExts.count(std::string(value.substr(extIndex + 1)))) + return ""; + } + } + + files.push_back(std::string(value)); + return ""; + }, + ""); + + CommandLine::ParseOptions parseOpts; + parseOpts.expandEnvVars = true; + parseOpts.ignoreProgramName = true; + parseOpts.supportComments = true; + parseOpts.ignoreDuplicates = true; + + if (!unitCmdLine.parse(text, parseOpts)) { + for (auto& err : unitCmdLine.getErrors()) + OS::printE(fmt::format("{}\n", err)); + return false; + } + + sourceLoader.addSeparateUnit(files, std::move(includes), std::move(defines), + std::move(libraryName).value_or(std::string())); + + return true; +} + void Driver::addLibraryFiles(std::string_view pattern) { // Parse the pattern; there's an optional leading library name // followed by an equals sign. If not there, we use the default diff --git a/source/driver/SourceLoader.cpp b/source/driver/SourceLoader.cpp index a73fd936a..0e6232c07 100644 --- a/source/driver/SourceLoader.cpp +++ b/source/driver/SourceLoader.cpp @@ -32,11 +32,13 @@ SourceLoader::SourceLoader(SourceManager& sourceManager) : sourceManager(sourceM void SourceLoader::addFiles(std::string_view pattern) { addFilesInternal(pattern, {}, /* isLibraryFile */ false, /* library */ nullptr, + /* unit */ nullptr, /* expandEnvVars */ false); } void SourceLoader::addLibraryFiles(std::string_view libName, std::string_view pattern) { addFilesInternal(pattern, {}, /* isLibraryFile */ true, getOrAddLibrary(libName), + /* unit */ nullptr, /* expandEnvVars */ false); } @@ -71,6 +73,21 @@ void SourceLoader::addLibraryMaps(std::string_view pattern, const fs::path& base addLibraryMapsInternal(pattern, basePath, optionBag, /* expandEnvVars */ false, seenMaps); } +void SourceLoader::addSeparateUnit(std::span filePatterns, + std::vector includePaths, + std::vector defines, std::string libraryName) { + auto& unit = unitEntries.emplace_back(); + unit.includePaths = std::move(includePaths); + unit.defines = std::move(defines); + unit.library = getOrAddLibrary(libraryName); + const bool isLibraryFile = unit.library != nullptr; + + for (auto pattern : filePatterns) { + addFilesInternal(pattern, {}, isLibraryFile, unit.library, &unit, + /* expandEnvVars */ false); + } +} + void SourceLoader::addLibraryMapsInternal(std::string_view pattern, const fs::path& basePath, const Bag& optionBag, bool expandEnvVars, flat_hash_set& seenMaps) { @@ -146,6 +163,7 @@ SourceLoader::SyntaxTreeList SourceLoader::loadAndParseSources(const Bag& option std::vector singleUnitBuffers; std::vector deferredLibBuffers; std::span inheritedMacros; + flat_hash_map> unitToBufferMap; const size_t fileEntryCount = fileEntries.size(); syntaxTrees.reserve(fileEntryCount); @@ -157,9 +175,12 @@ SourceLoader::SyntaxTreeList SourceLoader::loadAndParseSources(const Bag& option auto handleLoadResult = [&](LoadResult&& result) { switch (result.index()) { case 0: + // File was loaded and parsed independently. syntaxTrees.emplace_back(std::get<0>(std::move(result))); break; case 1: { + // File was loaded but it's a library file and we + // need to wait to include it in a parse operation. auto [buffer, isDeferredLib] = std::get<1>(result); if (isDeferredLib) deferredLibBuffers.push_back(buffer); @@ -168,10 +189,18 @@ SourceLoader::SyntaxTreeList SourceLoader::loadAndParseSources(const Bag& option break; } case 2: { + // Error occurred. auto [entry, code] = std::get<2>(result); addError(entry->path, code); break; } + case 3: { + // File is part of a separate unit. + auto [buffer, unit] = std::get<3>(result); + SLANG_ASSERT(unit != nullptr); + unitToBufferMap[unit].push_back(buffer); + break; + } } }; @@ -187,6 +216,13 @@ SourceLoader::SyntaxTreeList SourceLoader::loadAndParseSources(const Bag& option } }; + auto parseSeparateUnit = [&](const UnitEntry& unit, const std::vector& buffers) { + // TODO: handle other unit features + auto tree = SyntaxTree::fromBuffers(buffers, sourceManager, optionBag, inheritedMacros); + tree->isLibraryUnit = srcOptions.onlyLint || unit.library != nullptr; + return tree; + }; + if (fileEntries.size() >= MinFilesForThreading && srcOptions.numThreads != 1u) { // If there are enough files to parse and the user hasn't disabled // the use of threads, do the parsing via a thread pool. @@ -208,6 +244,25 @@ SourceLoader::SyntaxTreeList SourceLoader::loadAndParseSources(const Bag& option parseSingleUnit(singleUnitBuffers); + // Parse separate unit groups into their own syntax trees. + if (!unitToBufferMap.empty()) { + std::vector>*> unitList; + unitList.reserve(unitToBufferMap.size()); + for (auto& pair : unitToBufferMap) + unitList.push_back(&pair); + + const size_t numTrees = syntaxTrees.size(); + syntaxTrees.resize(numTrees + unitList.size()); + + threadPool.pushLoop(size_t(0), unitList.size(), [&](size_t start, size_t end) { + for (size_t i = start; i < end; i++) { + syntaxTrees[i + numTrees] = parseSeparateUnit(*unitList[i]->first, + unitList[i]->second); + } + }); + threadPool.waitForAll(); + } + // If we deferred libraries due to wanting to inherit macros, parse them now. if (!deferredLibBuffers.empty()) { const size_t numTrees = syntaxTrees.size(); @@ -234,6 +289,12 @@ SourceLoader::SyntaxTreeList SourceLoader::loadAndParseSources(const Bag& option parseSingleUnit(singleUnitBuffers); + // Parse separate unit groups into their own syntax trees. + if (!unitToBufferMap.empty()) { + for (auto& [unit, buffers] : unitToBufferMap) + syntaxTrees.emplace_back(parseSeparateUnit(*unit, buffers)); + } + // If we deferred libraries due to wanting to inherit macros, parse them now. if (!deferredLibBuffers.empty()) { for (auto& buffer : deferredLibBuffers) { @@ -363,7 +424,7 @@ SourceLibrary* SourceLoader::getOrAddLibrary(std::string_view name) { void SourceLoader::addFilesInternal(std::string_view pattern, const fs::path& basePath, bool isLibraryFile, const SourceLibrary* library, - bool expandEnvVars) { + const UnitEntry* unit, bool expandEnvVars) { SmallVector files; std::error_code ec; auto rank = svGlob(basePath, pattern, GlobMode::Files, files, expandEnvVars, ec); @@ -376,12 +437,20 @@ void SourceLoader::addFilesInternal(std::string_view pattern, const fs::path& ba for (auto&& path : files) { auto [it, inserted] = fileIndex.try_emplace(path, fileEntries.size()); if (inserted) { - fileEntries.emplace_back(std::move(path), isLibraryFile, library, rank); + fileEntries.emplace_back(std::move(path), isLibraryFile, library, unit, rank); } else { + // If this file is supposed to be in a separate unit but is already + // included elsewhere we should error. + auto& entry = fileEntries[it->second]; + if (unit || entry.unit) { + errors.emplace_back( + fmt::format("'{}': included in multiple compilation units", getU8Str(path))); + continue; + } + // If any of the times we see this is entry is for a non-library file, // then it's always a non-library file, hence the &=. - auto& entry = fileEntries[it->second]; entry.isLibraryFile &= isLibraryFile; if (library) { @@ -392,6 +461,7 @@ void SourceLoader::addFilesInternal(std::string_view pattern, const fs::path& ba if (!entry.library || rank < entry.libraryRank) { entry.library = library; entry.libraryRank = rank; + entry.secondLib = nullptr; } else if (rank == entry.libraryRank) { entry.secondLib = library; @@ -410,7 +480,7 @@ void SourceLoader::createLibrary(const LibraryDeclarationSyntax& syntax, const f for (auto filePath : syntax.filePaths) { auto spec = getPathFromSpec(*filePath); if (!spec.empty()) { - addFilesInternal(spec, basePath, /* isLibraryFile */ true, library, + addFilesInternal(spec, basePath, /* isLibraryFile */ true, library, nullptr, /* expandEnvVars */ true); } } @@ -445,7 +515,10 @@ SourceLoader::LoadResult SourceLoader::loadAndParse(const FileEntry& entry, cons if (!buffer) return std::pair{&entry, buffer.error()}; - if (!entry.isLibraryFile && srcOptions.singleUnit) { + if (entry.unit) { + return std::pair{*buffer, entry.unit}; + } + else if (!entry.isLibraryFile && srcOptions.singleUnit) { // If this file was directly specified (i.e. not via // a library mapping) and we're in single-unit mode, // collect it for later parsing. diff --git a/tests/unittests/DriverTests.cpp b/tests/unittests/DriverTests.cpp index 26dbd847b..d2793db6c 100644 --- a/tests/unittests/DriverTests.cpp +++ b/tests/unittests/DriverTests.cpp @@ -5,6 +5,8 @@ #include #include +#include "slang/ast/symbols/CompilationUnitSymbols.h" +#include "slang/ast/symbols/InstanceSymbols.h" #include "slang/driver/Driver.h" using namespace slang::driver; @@ -615,3 +617,32 @@ TEST_CASE("Driver checking for infinite library map includes") { CHECK(stderrContains("error: library map ")); CHECK(stderrContains("includes itself recursively")); } + +TEST_CASE("Driver separate unit listing") { + auto guard = OS::captureOutput(); + + Driver driver; + driver.addStandardArgs(); + + auto args = fmt::format("testfoo \"{0}test5.sv\" -C \"{0}unit.f\" -v \"lib2={0}test6.sv\"", + findTestDir()); + CHECK(driver.parseCommandLine(args)); + CHECK(driver.processOptions()); + CHECK(driver.parseAllSources()); + + auto compilation = driver.createCompilation(); + CHECK(driver.reportCompilation(*compilation, false)); + CHECK(stdoutContains("Build succeeded")); + + auto& root = compilation->getRoot(); + REQUIRE(root.topInstances.size() == 1); + CHECK(root.topInstances[0]->getSourceLibrary() == nullptr); + CHECK(root.topInstances[0]->name == "k"); + + auto units = compilation->getCompilationUnits(); + REQUIRE(units.size() == 3); + REQUIRE(units[1]->getSourceLibrary() != nullptr); + REQUIRE(units[2]->getSourceLibrary() != nullptr); + CHECK(units[1]->getSourceLibrary()->name == "lib2"); + CHECK(units[2]->getSourceLibrary()->name == "mylib"); +} diff --git a/tests/unittests/data/test5.sv b/tests/unittests/data/test5.sv index 21a779d64..24e3a0966 100644 --- a/tests/unittests/data/test5.sv +++ b/tests/unittests/data/test5.sv @@ -1,4 +1,4 @@ -module m; +module k; logic [3:0] a; logic [2:0] b = a; endmodule diff --git a/tests/unittests/data/unit.f b/tests/unittests/data/unit.f new file mode 100644 index 000000000..cc9278618 --- /dev/null +++ b/tests/unittests/data/unit.f @@ -0,0 +1,3 @@ +--library mylib +test.sv +test2.sv