[WIP] feat(codegen): Add runtime stride-based tensor offset computation and…#197
[WIP] feat(codegen): Add runtime stride-based tensor offset computation and…#197YunjiQin wants to merge 1 commit intohw-native-sys:mainfrom
Conversation
… codegen for tensor.dim Implements dynamic stride-based offset computation for CCE codegen, replacing compile-time row-major stride calculation with runtime stride access from Tensor struct. Key changes: - Add Tensor struct pointer tracking in CodeContext - Update block.load, block.store, block.l0c_store to use runtime strides - Add codegen function for tensor.dim op - Change GenerateStrideType to emit dynamic strides (-1) - Update tests to reflect dynamic stride behavior
Summary of ChangesHello @YunjiQin, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the CCE code generation framework by introducing robust support for dynamic tensor operations. It enables the system to handle variable tensor shapes and memory layouts more effectively at runtime through stride-based offset computations and the ability to query tensor dimensions. This change improves the flexibility and adaptability of the generated code for various tensor processing scenarios. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant feature for runtime stride-based tensor offset computation, which is crucial for handling dynamic tensor shapes in the CCE codegen. The changes are well-organized across the codegen engine, backend operator implementations, and context management. The implementation of tensor.dim and the updates to block.load/store to use the new dynamic offset calculation are solid. My review has identified a few minor areas for improvement, including removing unused variables/parameters, correcting documentation to match implementation, and addressing a potential signed/unsigned comparison issue. All comments align with existing guidelines or are not covered by specific rules, and thus no modifications or removals were necessary.
| static std::string ComputeStrideBasedOffset(codegen::CCECodegen& codegen, const std::string& tensor_var_name, | ||
| ir::MakeTuplePtr offsets, const ir::TensorTypePtr& tensor_type) { |
There was a problem hiding this comment.
The tensor_type parameter is unused within this function. It's good practice to remove unused parameters to keep the function signature clean and improve code clarity.
| static std::string ComputeStrideBasedOffset(codegen::CCECodegen& codegen, const std::string& tensor_var_name, | |
| ir::MakeTuplePtr offsets, const ir::TensorTypePtr& tensor_type) { | |
| static std::string ComputeStrideBasedOffset(codegen::CCECodegen& codegen, const std::string& tensor_var_name, | |
| ir::MakeTuplePtr offsets) { |
| static std::string MakeTensorDimCodegenCCE(const ir::CallPtr& op, codegen::CodegenBase& codegen_base) { | ||
| auto& codegen = dynamic_cast<codegen::CCECodegen&>(codegen_base); | ||
| std::string target_var = codegen.GetCurrentResultTarget(); | ||
| std::string input_var = codegen.GetExprAsCode(op->args_[0]); |
There was a problem hiding this comment.
The variable input_var is initialized but never used. It should be removed to avoid confusion and clean up the code.
| } | ||
| if (tensor_struct_ptr.has_value()) { | ||
| global_instance << ", {}, {"; | ||
| for (int i = 0; i < shape_dims.size(); i++) { |
There was a problem hiding this comment.
The loop counter i is of type int while shape_dims.size() returns size_t. This can lead to signed/unsigned comparison warnings from the compiler and is generally not safe. It's best practice to use size_t for loop counters that iterate over container sizes.
| for (int i = 0; i < shape_dims.size(); i++) { | |
| for (size_t i = 0; i < shape_dims.size(); i++) { |
| * Returns the Tensor struct pointer name that should be used for accessing | ||
| * buffer address and stride information. If no mapping exists, returns the | ||
| * input tensor_var_name itself (for compatibility). |
There was a problem hiding this comment.
The documentation for GetTensorStruct is inconsistent with its implementation. The documentation states that it returns the input tensor_var_name if no mapping exists, but the implementation in code_context.cpp throws an error using CHECK. The documentation should be updated to match the implementation's fail-fast behavior, which is safer.
| * Returns the Tensor struct pointer name that should be used for accessing | |
| * buffer address and stride information. If no mapping exists, returns the | |
| * input tensor_var_name itself (for compatibility). | |
| * Returns the Tensor struct pointer name that should be used for accessing | |
| * buffer address and stride information. Throws an error if no mapping exists. |
1 participant
Summary