Arch-aware Mapping (successfully built, read spec path from ENV) #101

Jackcuii · 2025-07-31T00:55:35Z

Just a draft up till now. can build, but has runtime bugs to be solved

Copilot

Pull Request Overview

This PR introduces architecture-aware mapping functionality by implementing YAML-based configuration for the hardware architecture specification. The implementation allows reading architecture details from an environment variable instead of using hardcoded values.

Added YAML configuration support for flexible architecture specification
Extended operation kind mappings to support more MLIR operations
Enhanced the MapToAcceleratorPass to read architecture from environment variables

Reviewed Changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
test/lit.cfg	Added LIT test configuration with hardcoded tool paths
lib/NeuraDialect/Transforms/MapToAcceleratorPass.cpp	Implemented YAML-based architecture loading from environment variable
lib/NeuraDialect/Mapping/mapping_util.cpp	Extended operation kind mapping to support additional MLIR operations
lib/NeuraDialect/CMakeLists.txt	Added yaml-cpp dependency and exception handling
lib/NeuraDialect/Architecture/Architecture.cpp	Implemented YAML constructor for Architecture class
include/NeuraDialect/Architecture/Architecture.h	Extended OperationKind enum and added YAML constructor declaration
CMakeLists.txt	Added yaml-cpp dependency and global exception handling

Copilot · 2025-07-31T00:59:12Z

test/lit.cfg

+config.substitutions.append(('mlir-neura-opt', '/home/jackcui/Arch/MLiR/dataflow/build/tools/mlir-neura-opt/mlir-neura-opt'))
+config.substitutions.append(('neura-interpreter', '/home/jackcui/Arch/MLiR/dataflow/build/tools/neura-interpreter/neura-interpreter'))
+config.substitutions.append(('neura-compiler', '/home/jackcui/Arch/MLiR/dataflow/build/tools/neura-compiler/neura-compiler'))
+config.substitutions.append(('FileCheck', '/home/jackcui/Arch/MLiR/llvm-project/build/./bin/FileCheck'))
+config.substitutions.append(('mlir-opt', '/home/jackcui/Arch/MLiR/llvm-project/build/./bin/mlir-opt'))
+config.substitutions.append(('mlir-translate', '/home/jackcui/Arch/MLiR/llvm-project/build/./bin/mlir-translate'))
+config.substitutions.append(('llc', '/home/jackcui/Arch/MLiR/llvm-project/build/./bin/llc'))
+config.substitutions.append(('clang', '/home/jackcui/Arch/MLiR/llvm-project/build/./bin/clang'))


Hardcoded absolute paths make the configuration non-portable across different development environments. Consider using relative paths or environment variables to make this configuration more flexible.

Suggested change

config.substitutions.append(('mlir-neura-opt', '/home/jackcui/Arch/MLiR/dataflow/build/tools/mlir-neura-opt/mlir-neura-opt'))

config.substitutions.append(('neura-interpreter', '/home/jackcui/Arch/MLiR/dataflow/build/tools/neura-interpreter/neura-interpreter'))

config.substitutions.append(('neura-compiler', '/home/jackcui/Arch/MLiR/dataflow/build/tools/neura-compiler/neura-compiler'))

config.substitutions.append(('FileCheck', '/home/jackcui/Arch/MLiR/llvm-project/build/./bin/FileCheck'))

config.substitutions.append(('mlir-opt', '/home/jackcui/Arch/MLiR/llvm-project/build/./bin/mlir-opt'))

config.substitutions.append(('mlir-translate', '/home/jackcui/Arch/MLiR/llvm-project/build/./bin/mlir-translate'))

config.substitutions.append(('llc', '/home/jackcui/Arch/MLiR/llvm-project/build/./bin/llc'))

config.substitutions.append(('clang', '/home/jackcui/Arch/MLiR/llvm-project/build/./bin/clang'))

config.substitutions.append(('mlir-neura-opt', os.getenv('MLIR_NEURA_OPT_PATH', '')))

config.substitutions.append(('neura-interpreter', os.getenv('NEURA_INTERPRETER_PATH', '')))

config.substitutions.append(('neura-compiler', os.getenv('NEURA_COMPILER_PATH', '')))

config.substitutions.append(('FileCheck', os.getenv('FILECHECK_PATH', '')))

config.substitutions.append(('mlir-opt', os.getenv('MLIR_OPT_PATH', '')))

config.substitutions.append(('mlir-translate', os.getenv('MLIR_TRANSLATE_PATH', '')))

config.substitutions.append(('llc', os.getenv('LLC_PATH', '')))

config.substitutions.append(('clang', os.getenv('CLANG_PATH', '')))

We shouldn't check in this file. Similarly, plz remove all the build files. avoid using git add .

Copilot · 2025-07-31T00:59:12Z

lib/NeuraDialect/Transforms/MapToAcceleratorPass.cpp

+          } else {
+            llvm::errs() << "[MapToAcceleratorPass] Could not open architecture file " 
+                        << arch_spec_path << ", using default 4x4\n";
+            use_default_arch = true;


The variable use_default_arch is set to true when the file cannot be opened, but this assignment is redundant since use_default_arch is already initialized to false and should remain false to trigger the YAML-based architecture creation.

Suggested change

use_default_arch = true;

Copilot · 2025-07-31T00:59:12Z

lib/NeuraDialect/Transforms/MapToAcceleratorPass.cpp

+          }
+        } catch (const std::exception& e) {
+          llvm::errs() << "[MapToAcceleratorPass] Error parsing YAML file " 
+                      << arch_spec_path << ": " << e.what() << ", using default 4x4\n";


When YAML parsing fails, the code should set use_default_arch = true to ensure the default architecture is used, but this assignment is missing.

Suggested change

<< arch_spec_path << ": " << e.what() << ", using default 4x4\n";

<< arch_spec_path << ": " << e.what() << ", using default 4x4\n";

use_default_arch = true;

Copilot · 2025-07-31T00:59:13Z

lib/NeuraDialect/Architecture/Architecture.cpp

+Architecture::Architecture(const YAML::Node& config) {
+  // Extract width and height from config
+  int width = 4;  // default
+  int height = 4; // default
+
+  if (config["architecture"] && config["architecture"]["width"] && config["architecture"]["height"]) {
+    width = config["architecture"]["width"].as<int>();
+    height = config["architecture"]["height"].as<int>();
+  }
+
+  // Call the constructor with width and height.
+  *this = Architecture(width, height);
+
+  // Add function units based on the architecture specs.
+  int num_tiles = width * height;


Using assignment operator on *this in a constructor is problematic and can lead to undefined behavior. Consider using constructor delegation or member initialization instead.

Suggested change

Architecture::Architecture(const YAML::Node& config) {

// Extract width and height from config

int width = 4; // default

int height = 4; // default

if (config["architecture"] && config["architecture"]["width"] && config["architecture"]["height"]) {

width = config["architecture"]["width"].as<int>();

height = config["architecture"]["height"].as<int>();

}

// Call the constructor with width and height.

*this = Architecture(width, height);

// Add function units based on the architecture specs.

int num_tiles = width * height;

Architecture::Architecture(const YAML::Node& config)

: Architecture(

config["architecture"] && config["architecture"]["width"] && config["architecture"]["height"]

? config["architecture"]["width"].as<int>()

: 4,

config["architecture"] && config["architecture"]["height"]

? config["architecture"]["height"].as<int>()

: 4) {

// Add function units based on the architecture specs.

int num_tiles = getNumTiles();

Copilot · 2025-07-31T00:59:13Z

lib/NeuraDialect/Architecture/Architecture.cpp

+      // Override the default function units.
+      for (const auto& operation : config["tile_overrides"][i]["operations"]) {
+        if (operation.as<std::string>() == "add") {
+          tile->addFunctionUnit(std::make_unique<FixedPointAdder>(++fu_id));


The variable fu_id is pre-incremented before use, which means the first function unit will have ID 1 instead of 0. This inconsistency could cause issues if function unit IDs are expected to start from 0.

Suggested change

tile->addFunctionUnit(std::make_unique<FixedPointAdder>(++fu_id));

tile->addFunctionUnit(std::make_unique<FixedPointAdder>(fu_id++));

I think copilot's suggestion makes sense?

Copilot · 2025-07-31T00:59:13Z

lib/NeuraDialect/Architecture/Architecture.cpp

+      // Add default function units.
+      for (const auto& operation : config["tile_defaults"]["operations"]) {
+        if (operation.as<std::string>() == "add") {
+          tile->addFunctionUnit(std::make_unique<FixedPointAdder>(++fu_id));


Same issue as above - fu_id should be post-incremented or the logic should be adjusted to start from 0.

Suggested change

tile->addFunctionUnit(std::make_unique<FixedPointAdder>(++fu_id));

tile->addFunctionUnit(std::make_unique<FixedPointAdder>(fu_id++));

Copilot · 2025-07-31T00:59:14Z

include/NeuraDialect/Architecture/Architecture.h

+  FMulFAdd = 9,
+  VFMul = 10,
+  ICmp = 11,
+  FCmp = 12,


The FCmp operation is defined in the enum but not handled in the getOperationKindFromMlirOp function in mapping_util.cpp, which could lead to incorrect operation mapping.

tancheng · 2025-07-31T04:32:01Z

@Jackcuii is this PR ready for review? If yes, @ShangkunLi plz help review. Otherwise, plz first make this PR pass github action.

Jackcuii · 2025-07-31T18:08:20Z

@Jackcuii is this PR ready for review? If yes, @ShangkunLi plz help review. Otherwise, plz first make this PR pass github action.

Hi Sir Tan, it seems I need some more time on it before review. And maybe we should adapt the Github Action to install the yaml library.

tancheng · 2025-08-04T04:38:51Z

@Jackcuii is this PR ready for review? If yes, @ShangkunLi plz help review. Otherwise, plz first make this PR pass github action.

Hi Sir Tan, it seems I need some more time on it before review. And maybe we should adapt the Github Action to install the yaml library.

CCing @n0thingNoob

I saw instruction.json provided by @n0thingNoob, but here you are relying on yaml. So do we really need two formats?

Jackcuii · 2025-08-05T18:53:38Z

@Jackcuii is this PR ready for review? If yes, @ShangkunLi plz help review. Otherwise, plz first make this PR pass github action.

Hi Sir Tan, it seems I need some more time on it before review. And maybe we should adapt the Github Action to install the yaml library.

CCing @n0thingNoob

I saw instruction.json provided by @n0thingNoob, but here you are relying on yaml. So do we really need two formats?

The yaml here is the description of architecture.
Accroding to my understanding, the instruction.json is the generated instruction sequence

tancheng · 2025-08-05T19:06:59Z

@Jackcuii is this PR ready for review? If yes, @ShangkunLi plz help review. Otherwise, plz first make this PR pass github action.

Hi Sir Tan, it seems I need some more time on it before review. And maybe we should adapt the Github Action to install the yaml library.

CCing @n0thingNoob
I saw instruction.json provided by @n0thingNoob, but here you are relying on yaml. So do we really need two formats?

The yaml here is the description of architecture. Accroding to my understanding, the instruction.json is the generated instruction sequence

My question is can we only use one format for all? Each format would increase the codebase by introducing library and header files.

n0thingNoob · 2025-08-06T01:37:28Z

@Jackcuii is this PR ready for review? If yes, @ShangkunLi plz help review. Otherwise, plz first make this PR pass github action.

Hi Sir Tan, it seems I need some more time on it before review. And maybe we should adapt the Github Action to install the yaml library.

CCing @n0thingNoob
I saw instruction.json provided by @n0thingNoob, but here you are relying on yaml. So do we really need two formats?

The yaml here is the description of architecture. Accroding to my understanding, the instruction.json is the generated instruction sequence

My question is can we only use one format for all? Each format would increase the codebase by introducing library and header files.

Sorry for the late reply, we can just use yaml. I can delete the instruction.json later.

n0thingNoob · 2025-08-06T01:38:52Z

Do I also need to change the codegenpass? Since it is also generating json file.

tancheng · 2025-08-06T01:49:32Z

Do I also need to change the codegenpass? Since it is also generating json file.

Sure, if that's not hard for you. And it seems our arch spec is also having both yaml and json, so we can remove json there as well?

tancheng · 2025-08-07T00:47:17Z

.gitignore

Don't check in this file.

CMakeLists.txt

tancheng · 2025-08-07T00:50:16Z

include/NeuraDialect/Architecture/Architecture.h

+  Constant = 30,
+  DataMov = 31,
+  CtrlMov = 32,
+  Reserve = 33


We don't actually map Reserve, in other words, Reserve is just a placeholder to facilitate mapping, no HW needed to support it. Similarly, DataMov and CtrlMov are data movement, which would be data path traversing links/channels, not sure we need them or not.

I keep Data/CtrlMov temporarily then.

tancheng · 2025-08-07T00:50:45Z

include/NeuraDialect/Architecture/Architecture.h

+  Br_ = 22,
+  CondBr_ = 23,


Why we need _? Br and CondBr are reserved in C++?

It seems that it will conflict with the MLIR library built-in if we do not use '_'? The compiler feedback is not quite informative.

What about OpBr and OpCondBr? i.e., Op prefix for all.

lib/NeuraDialect/Architecture/Architecture.cpp

tancheng · 2025-08-07T00:55:07Z

lib/NeuraDialect/Architecture/Architecture.cpp

+      // Override the default function units.
+      for (const auto& operation : config["tile_overrides"][i]["operations"]) {
+        if (operation.as<std::string>() == "add") {
+          tile->addFunctionUnit(std::make_unique<FixedPointAdder>(++fu_id));


I think copilot's suggestion makes sense?

tancheng · 2025-08-07T00:55:13Z

lib/NeuraDialect/Architecture/Architecture.cpp

+      // Add default function units.
+      for (const auto& operation : config["tile_defaults"]["operations"]) {
+        if (operation.as<std::string>() == "add") {
+          tile->addFunctionUnit(std::make_unique<FixedPointAdder>(++fu_id));


tancheng · 2025-08-07T00:57:09Z

lib/NeuraDialect/Transforms/MapToAcceleratorPass.cpp

+      // Read architecture specification from command line option
+      YAML::Node config;
+      bool use_default_arch = false;
+
+      if (!archSpecPath.getValue().empty()) {
+        try {
+          std::ifstream file(archSpecPath.getValue());
+          if (file.is_open()) {
+            config = YAML::Load(file);
+            if (config["architecture"]) {
+              llvm::outs() << "\033[31m[MapToAcceleratorPass] Loaded architecture from " 
+                          << archSpecPath.getValue() << "\033[0m\n";
+            } else {
+              llvm::errs() << "[MapToAcceleratorPass] Invalid YAML format in " 
+                          << archSpecPath.getValue() << ", using default 4x4\n";
+              use_default_arch = true;
+            }
+          } else {
+            llvm::errs() << "[MapToAcceleratorPass] Could not open architecture file " 
+                        << archSpecPath.getValue() << ", using default 4x4\n";
+            use_default_arch = true;
+          }
+        } catch (const std::exception& e) {
+          llvm::errs() << "[MapToAcceleratorPass] Error parsing YAML file " 
+                      << archSpecPath.getValue() << ": " << e.what() << ", using default 4x4\n";
+          use_default_arch = true;
+        }
+      } else {
+        use_default_arch = true;
+        llvm::errs() << "[MapToAcceleratorPass] No architecture specification provided, using default 4x4\n";
+      }


Put these into a function?

tancheng · 2025-08-07T00:59:42Z

lib/NeuraDialect/Transforms/MapToAcceleratorPass.cpp

+        llvm::errs() << "[MapToAcceleratorPass] No architecture specification provided, using default 4x4\n";
+      }
+
+      Architecture architecture = use_default_arch ? Architecture(4, 4) : Architecture(config);


constexpr int kWidth = 4; constexpr int kHeight = 4; Architecture architecture = use_default_arch ? Architecture(kWidth, kHeight) : Architecture(config);

All the problems above are fixed, but I can still not reproduce the environment problem. (even in a locally deployed Docker) a bit strange

You mean yaml-related env problem cannot be fixed?

-- Configuring incomplete, errors occurred! By not providing "Findyaml-cpp.cmake" in CMAKE_MODULE_PATH this project has asked CMake to find a package configuration file provided by "yaml-cpp", but CMake did not find one. Could not find a package configuration file provided by "yaml-cpp" with any of the following names: yaml-cppConfig.cmake yaml-cpp-config.cmake Add the installation prefix of "yaml-cpp" to CMAKE_PREFIX_PATH or set "yaml-cpp_DIR" to a directory containing one of the above files. If "yaml-cpp" provides a separate development package or SDK, be sure it has been installed.

yep. The main problem is that even with a totally clean Docker env, the problem does not occurs. And I am not sure what the real path of them in a blackbox workflow env.

I saw some one said libyaml-cpp-dev (instead of libyaml-dev) may work. Let me try.

You mean yaml-related env problem cannot be fixed?

-- Configuring incomplete, errors occurred! By not providing "Findyaml-cpp.cmake" in CMAKE_MODULE_PATH this project has asked CMake to find a package configuration file provided by "yaml-cpp", but CMake did not find one. Could not find a package configuration file provided by "yaml-cpp" with any of the following names: yaml-cppConfig.cmake yaml-cpp-config.cmake Add the installation prefix of "yaml-cpp" to CMAKE_PREFIX_PATH or set "yaml-cpp_DIR" to a directory containing one of the above files. If "yaml-cpp" provides a separate development package or SDK, be sure it has been installed.

Cool. It seems to work! (Still something to be fixed :D)

update workflow to enable yaml cmake finding

ShangkunLi · 2025-08-11T05:42:53Z

lib/NeuraDialect/Mapping/mapping_util.cpp

+  if (isa<neura::GrantAlwaysOp>(op)) return OpGrantAlways;
+  if (isa<neura::GrantOnceOp>(op)) return OpGrantOnce;
+  if (isa<neura::GrantPredicateOp>(op)) return OpGrantPredicate;
+  if (isa<neura::GEP>(op))      return OpGEP_;


Why this line use OpGEP_ instead of OpGEP?

Oh. My mistake. should use OpGEP

successfully built, read spec path from ENV

30187d6

Jackcuii changed the title ~~successfully built, read spec path from ENV~~ Arch-aware Mapping (successfully built, read spec path from ENV) Jul 31, 2025

Jackcuii requested review from ShangkunLi, Copilot and tancheng July 31, 2025 00:57

Copilot AI reviewed Jul 31, 2025

View reviewed changes

Update main.yml to install yaml lib

db8d764

Jackcui and others added 3 commits August 6, 2025 12:40

edit gitignore, reading spec from ENV -> cli parameters

db24510

fix conflict

38abb68

Merge branch 'main' into add-arch-aware-mapping

7333452

tancheng reviewed Aug 7, 2025

View reviewed changes

Jackcui and others added 3 commits August 10, 2025 19:50

fix for PR

2609389

merge diff

deef1c0

Update main.yml

124f8cb

update workflow to enable yaml cmake finding

ShangkunLi reviewed Aug 11, 2025

View reviewed changes

	<< arch_spec_path << ": " << e.what() << ", using default 4x4\n";
	<< arch_spec_path << ": " << e.what() << ", using default 4x4\n";
	use_default_arch = true;

	tile->addFunctionUnit(std::make_unique<FixedPointAdder>(++fu_id));
	tile->addFunctionUnit(std::make_unique<FixedPointAdder>(fu_id++));

Arch-aware Mapping (successfully built, read spec path from ENV) #101

Are you sure you want to change the base?

Arch-aware Mapping (successfully built, read spec path from ENV) #101

Uh oh!

Conversation

Jackcuii commented Jul 31, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

tancheng commented Jul 31, 2025

Uh oh!

Jackcuii commented Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tancheng commented Aug 4, 2025

Uh oh!

Jackcuii commented Aug 5, 2025

Uh oh!

tancheng commented Aug 5, 2025

Uh oh!

n0thingNoob commented Aug 6, 2025

Uh oh!

n0thingNoob commented Aug 6, 2025

Uh oh!

tancheng commented Aug 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jackcuii commented Jul 31, 2025 •

edited

Loading