refac: change arith constant op (merged with #61) #40
Conversation
ashjeong
force-pushed
the
refac/change-arith-constant-op
branch
from
1b164dd to
0bf058a
Compare
|
Closing this PR for now, as it's not a priority at the moment. We'll reopen it when the time is right |
ashjeong
force-pushed
the
refac/change-arith-constant-op
branch
from
0bf058a to
4a62cfa
Compare
ashjeong
force-pushed
the
refac/change-arith-constant-op
branch
from
4a62cfa to
a209db1
Compare
|
|
/gemini review |
![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 primarily refactors the codebase to align with upstream MLIR API changes, namely replacing arith::ConstantOp with the more direct arith::ConstantIntOp and renaming bufferization::ToMemrefOp to bufferization::ToBufferOp. The changes are applied consistently across the codebase, including tests and patch files for dependencies. A notable improvement is the more precise verification logic for the elliptic_curve.msm operation, which is now correctly documented. Overall, the refactoring is clean and well-executed. I've added a couple of suggestions to improve robustness by replacing assert(false, ...) with llvm_unreachable.
zkir/Dialect/EllipticCurve/Conversions/EllipticCurveToField/PointOperations/Jacobian/Add.cpp
Outdated
Show resolved
Hide resolved
zkir/Dialect/EllipticCurve/Conversions/EllipticCurveToField/PointOperations/XYZZ/Add.cpp
Outdated
Show resolved
Hide resolved
Comment has an error and did not reflect actual structure of code logic and so was fixed
ashjeong
force-pushed
the
refac/change-arith-constant-op
branch
from
a209db1 to
5c76cc2
Compare
Note that `ToMemref` has been changed to `ToBuffer` LLVM commit used contains the PRs - llvm/llvm-project#144636 - llvm/llvm-project#144638 which were committed in order to apply changes from `ConstantOp` to `ConstantIntOp`
Patch done to llvm-project: - Switches int/type ordering of `ConstantIntOp`to standardize with `ConstantOp` argument ordering - Adds constructor for `APInt` types Note that a `TypedAttr` input still requires `ConstantOp` to be used
patches/ is not a real folder and our patch files are all included in third-party/llvm-project/
ashjeong
force-pushed
the
refac/change-arith-constant-op
branch
from
5c76cc2 to
e2ac967
Compare
Without this move, pre-commit may need to be run twice
ashjeong
force-pushed
the
refac/change-arith-constant-op
branch
from
e2ac967 to
b3adf94
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a significant refactoring effort to align with upstream MLIR/LLVM API changes. The primary changes involve replacing arith::ConstantOp with arith::ConstantIntOp and bufferization::ToMemrefOp with bufferization::ToBufferOp. These changes have been applied consistently across the codebase. Additionally, there are several good practice improvements, such as replacing assert with llvm_unreachable and enhancing operator verification logic. The configuration for clang-tidy has also been updated to use a shell script, which is a good approach for managing platform-specific arguments. I've found one issue in this new script that needs to be addressed. Overall, the changes are well-executed and improve the codebase.
run_clang_tidy.sh
Outdated
|
|
||
| # Get the path to the directory containing this script. | ||
| SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )" | ||
| PROJECT_ROOT=$(dirname "$SCRIPT_DIR") |
There was a problem hiding this comment.
The PROJECT_ROOT variable appears to be defined incorrectly. SCRIPT_DIR will resolve to the project's root directory, and dirname "$SCRIPT_DIR" will then point to the parent directory of the project. This will likely cause clang-tidy to fail to find compile_commands.json, which is expected to be at the project root.
| PROJECT_ROOT=$(dirname "$SCRIPT_DIR") | |
| PROJECT_ROOT="$SCRIPT_DIR" |
ashjeong
force-pushed
the
refac/change-arith-constant-op
branch
from
b3adf94 to
2fdd49b
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a large refactoring that primarily replaces arith::ConstantOp with the more specific arith::ConstantIntOp where applicable. This helps reduce code clutter by avoiding the creation of extraneous IntegerAttr. The changes also include replacing bufferization::ToMemrefOp with bufferization::ToBufferOp to align with upstream MLIR changes.
Beyond the main refactoring, the PR introduces a new script for clang-tidy in pre-commit hooks, updates LLVM dependencies, and improves code quality by replacing assert with llvm_unreachable for better compiler optimizations.
The changes are extensive but look correct and consistent with the stated goals. I've added a couple of suggestions to make the new run_clang_tidy.sh script more robust.
ashjeong
force-pushed
the
refac/change-arith-constant-op
branch
5 times, most recently
from
6e14d69 to
bf7d6e0
Compare
ashjeong
force-pushed
the
refac/change-arith-constant-op
branch
10 times, most recently
from
2a246d9 to
a598ee9
Compare
ashjeong
force-pushed
the
refac/change-arith-constant-op
branch
from
a598ee9 to
e71eef0
Compare
ashjeong
changed the title
ashjeong
changed the title
Replaces
ConstantOpwithConstantIntOpwhere applicable to reduce code clutter of creating extraneousIntegerAttr.Relies on
llvm/llvm-project#144636 & llvm/llvm-project#144638
Note that
ConstantIntOpdoes not support a singleTypedAttrargument or arguments of container types and values – continue to useConstantOpfor these cases.Please merge with respective PR in ZKX!
ONLY merge when the CI works properlyThis PR was mistakenly merged with #61 to main, while CI was still not finishing successfully. See #62 for the fix.