Skip to content

Code Generate Pass#88

Merged
n0thingNoob merged 60 commits intomainfrom
architecture_spec
Sep 8, 2025
Merged

Code Generate Pass#88
n0thingNoob merged 60 commits intomainfrom
architecture_spec

Conversation

@n0thingNoob
Copy link
Copy Markdown
Collaborator

@n0thingNoob n0thingNoob commented Jul 20, 2025

Current work

  • Fixed source direction calculation for operations with data_mov operands
  • Added op_to_source_tile mapping to track original source locations
  • Enhanced ASM generation with correct Entry conditions and operand directions
  • Added test results in the Generated_Code folder

Future Enhancements Needed

  • Add comprehensive error handling and validation
  • Refactor duplicate code in JSON/ASM generation
  • Add configuration options for output customization
  • Implement unit tests and integration tests
  • Support more complex data flow patterns
  • Add debugging and diagnostic capabilities

Testing

  • Tested with test/test.mlir, the results of asm and json look right, but might be missing some details.

Files Changed

  • lib/NeuraDialect/Transforms/GenerateCodePass.cpp
  • Generated_Code/ (new folder with test results)

- Add PE ID system with PE(id) as keys
- Organize instructions by PE coordinates (x, y)
- Calculate data movement directions (N, S, E, W, NE, SE, NW, SW, Local)
- Include complete instruction information (opcode, time_step, src_tile, dst_tile, operands)
- Support constant value extraction
- Generate structured JSON output for PE instructions
- Add example JSON output showing PE-based instruction organization
- Demonstrates the structure with PE(id) keys and coordinate information
- Shows complete instruction details including directions and time steps
- Serves as reference for the code generation pass output format
@n0thingNoob n0thingNoob changed the title Architecture spec Code Generate Pass Jul 20, 2025
@tancheng
Copy link
Copy Markdown
Contributor

@ShangkunLi any idea why the build time out? no changes related to mapper in this PR.

@n0thingNoob
Copy link
Copy Markdown
Collaborator Author

n0thingNoob commented Jul 21, 2025

Great, I still need more time to fix those errors. And I need to discuss some issue with @Jackcuii

@ShangkunLi
Copy link
Copy Markdown
Collaborator

@ShangkunLi any idea why the build time out? no changes related to mapper in this PR.

@n0thingNoob @tancheng I built and ran the code in this branch locally, the segment faults seem to come from the --generate-code pass.

@n0thingNoob
Copy link
Copy Markdown
Collaborator Author

@ShangkunLi any idea why the build time out? no changes related to mapper in this PR.

@n0thingNoob @tancheng I built and ran the code in this branch locally, the segment faults seem to come from the --generate-code pass.

ok, let me check what happened.

@n0thingNoob
Copy link
Copy Markdown
Collaborator Author

@ShangkunLi any idea why the build time out? no changes related to mapper in this PR.

@n0thingNoob @tancheng I built and ran the code in this branch locally, the segment faults seem to come from the --generate-code pass.

ok, let me check what happened.

@ShangkunLi How can I run the same test locally? I want to see what happened to my code

@tancheng
Copy link
Copy Markdown
Contributor

@ShangkunLi any idea why the build time out? no changes related to mapper in this PR.

@n0thingNoob @tancheng I built and ran the code in this branch locally, the segment faults seem to come from the --generate-code pass.

ok, let me check what happened.

@ShangkunLi How can I run the same test locally? I want to see what happened to my code

$ ../build/tools/mlir-neura-opt/mlir-neura-opt --debug test.mlir
and include all the needed passes:
// RUN: --assign-accelerator \
// RUN: --lower-llvm-to-neura \
// RUN: --leverage-predicated-value \
// RUN: --transform-ctrl-to-data-flow \
// RUN: --insert-data-mov \
// RUN: --map-to-accelerator="mapping-strategy=heuristic" \
// RUN: --generate-code

@n0thingNoob
Copy link
Copy Markdown
Collaborator Author

Looks like my revision passed. I can keep working on other issues.

@tancheng
Copy link
Copy Markdown
Contributor

Looks like my revision passed. I can keep working on other issues.

You still need to check your functionality in some tests like this

// RUN: mlir-neura-opt %s \
// RUN: --assign-accelerator \
// RUN: --lower-llvm-to-neura \
// RUN: --leverage-predicated-value \
// RUN: --transform-ctrl-to-data-flow \
// RUN: --insert-data-mov \
// RUN: --map-to-accelerator="mapping-strategy=heuristic" \
// RUN: --generate-code
// RU: FileCheck %s --input-file=generated-instructions.json -check-prefix=INST

@n0thingNoob
Copy link
Copy Markdown
Collaborator Author

Looks like my revision passed. I can keep working on other issues.

You still need to check your functionality in some tests like this

// RUN: mlir-neura-opt %s \
// RUN: --assign-accelerator \
// RUN: --lower-llvm-to-neura \
// RUN: --leverage-predicated-value \
// RUN: --transform-ctrl-to-data-flow \
// RUN: --insert-data-mov \
// RUN: --map-to-accelerator="mapping-strategy=heuristic" \
// RUN: --generate-code
// RU: FileCheck %s --input-file=generated-instructions.json -check-prefix=INST

Yes, I am working on that.

@n0thingNoob
Copy link
Copy Markdown
Collaborator Author

Updates: removed redundant IR walk() calls and unified traversal into a single function-level pass. Also standardized variable and function names (need reviews) @tancheng

Copy link
Copy Markdown
Contributor

@tancheng tancheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job Yijia~!

Plz fix the comments style and variable naming convention. Also, plz let me click on the "solve" once you fix all of them. Thanks.

@n0thingNoob
Copy link
Copy Markdown
Collaborator Author

n0thingNoob commented Sep 6, 2025

Great job Yijia~!

Plz fix the comments style and variable naming convention. Also, plz let me click on the "solve" once you fix all of them. Thanks.

Will do it ASAP. And for the last question:
Because not all operations have register mappings. The caller already handles this case properly with if (auto mapped_register_id = getMappedRegId(op)). We can add debug info here if necessary

@n0thingNoob
Copy link
Copy Markdown
Collaborator Author

I am not sure why there is a conflict when pushing the first commit. I merged the latest main into architecture_spec to resolve it and removed the obsolete README that was only used by the old test. @tancheng

@tancheng
Copy link
Copy Markdown
Contributor

tancheng commented Sep 6, 2025

I am not sure why there is a conflict when pushing the first commit. I merged the latest main into architecture_spec to resolve it and removed the obsolete README that was only used by the old test. @tancheng

I didn't get it. Will you provide a README or not?

@n0thingNoob
Copy link
Copy Markdown
Collaborator Author

I am not sure why there is a conflict when pushing the first commit. I merged the latest main into architecture_spec to resolve it and removed the obsolete README that was only used by the old test. @tancheng

I didn't get it. Will you provide a README or not?

Perhaps not, the branch test is a little bit complicated. Is the README necessary for this test?

@tancheng
Copy link
Copy Markdown
Contributor

tancheng commented Sep 7, 2025

I am not sure why there is a conflict when pushing the first commit. I merged the latest main into architecture_spec to resolve it and removed the obsolete README that was only used by the old test. @tancheng

I didn't get it. Will you provide a README or not?

Perhaps not, the branch test is a little bit complicated. Is the README necessary for this test?

Hi @n0thingNoob, I just meant to have simple explanation maybe about only one tile for two time steps. You don't need explain what happen on all the tiles across all time steps. How does this sound? You can put your README.md back in your test folder, like what @HobbitQia (https://github.com/coredac/dataflow/tree/main/test/visualize) and @ShangkunLi (https://github.com/coredac/dataflow/tree/main/test) did. Sounds good to you?

@n0thingNoob
Copy link
Copy Markdown
Collaborator Author

I am not sure why there is a conflict when pushing the first commit. I merged the latest main into architecture_spec to resolve it and removed the obsolete README that was only used by the old test. @tancheng

I didn't get it. Will you provide a README or not?

Perhaps not, the branch test is a little bit complicated. Is the README necessary for this test?

Hi @n0thingNoob, I just meant to have simple explanation maybe about only one tile for two time steps. You don't need explain what happen on all the tiles across all time steps. How does this sound? You can put your README.md back in your test folder, like what @HobbitQia (https://github.com/coredac/dataflow/tree/main/test/visualize) and @ShangkunLi (https://github.com/coredac/dataflow/tree/main/test) did. Sounds good to you?

Sure.

@tancheng
Copy link
Copy Markdown
Contributor

tancheng commented Sep 7, 2025

You are aware of your always failed test, right? https://github.com/coredac/dataflow/actions/runs/17523525173/job/49770978606

And it should be an easy fix for you I think.

@n0thingNoob
Copy link
Copy Markdown
Collaborator Author

You are aware of your always failed test, right? https://github.com/coredac/dataflow/actions/runs/17523525173/job/49770978606

And it should be an easy fix for you I think.

Working on it.

@tancheng
Copy link
Copy Markdown
Contributor

tancheng commented Sep 8, 2025

okay to merge now @n0thingNoob? or anything else you want to fix?

@n0thingNoob n0thingNoob merged commit 65911ff into main Sep 8, 2025
1 check passed
@n0thingNoob
Copy link
Copy Markdown
Collaborator Author

okay to merge now @n0thingNoob? or anything else you want to fix?

Yes, let's do it for now.

ShangkunLi pushed a commit that referenced this pull request Mar 12, 2026
ShangkunLi pushed a commit that referenced this pull request Mar 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

new feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants