-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[mlir][python] automatic location inference #151246
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
61b0669 to
9ea8751
Compare
|
✅ With the latest revision this PR passed the Python code formatter. |
34d5f14 to
7c53bf1
Compare
389db39 to
f63eaf2
Compare
2297849 to
a4bc05a
Compare
| if _cext.ir.Location.current: | ||
| return _cext.ir.Location.current.context | ||
| return None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized since we're now supporting loc inference we shouldn't require a location to be in the context; specifically Location.current shouldn't throw but just return None. See tests in context_manager.py below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jpienaar @rolfmorel i'm asking for a re-review because I made this change which wasn't in the original PR and slightly changes semantics; see also the change to Location.current here below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM
mlir/lib/Bindings/Python/IRCore.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change Location.current to return None instead of throwing - see above
mlir/lib/Bindings/Python/IRModule.h
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drive-by DCE (unused/no defn)
b0ff949 to
70244f3
Compare
…d fix warning unused Introduced (but omitted from this CMake) in #151246.
…y nanobind (#153861) Introduced (but omitted from this CMake) in llvm/llvm-project#151246.
…(#153861) Introduced (but omitted from this CMake) in llvm/llvm-project#151246.
…(#153861) Introduced (but omitted from this CMake) in llvm/llvm-project#151246.
|
Hi, I'm seeing the below error on macos build: I haven't tracked the change yet, but I'll try to provide a patch. Currently, we can only revert three commits together, otherwsie there are conflicts. |
|
Oh, 6fc1deb seems to fix the issue. Let me give it a shot. |
|
Yes, confirmed that it is fixed by the commit. Thanks |
| two = arith.constant(IndexType.get(), 2) | ||
|
|
||
| # fmt: off | ||
| # CHECK: loc(callsite("testInferLocations"("{{.*}}[[SEP:[/\\]]]test[[SEP]]python[[SEP]]ir[[SEP]]auto_location.py":31:13 to :43) at callsite("run"("{{.*}}[[SEP]]test[[SEP]]python[[SEP]]ir[[SEP]]auto_location.py":13:4 to :7) at "<module>"("{{.*}}[[SEP]]test[[SEP]]python[[SEP]]ir[[SEP]]auto_location.py":26:1 to :4)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recently faced the following error: ******************** TEST 'MLIR :: python/ir/auto_location.py' FAILED ******************** when LLVM was compiled for windows in OpenAI Triton project: https://github.com/triton-lang/triton/actions/runs/17419518063/job/49454994952.
More logs:
******************** TEST 'MLIR :: python/ir/auto_location.py' FAILED ********************
Exit Code: 1
Command Output (stdout):
--
# RUN: at line 1
"C:/hostedtoolcache/windows/Python/3.11.9/x64/python3.exe" D:\a\triton\triton\llvm-project\mlir\test\python\ir\auto_location.py | d:\a\triton\triton\llvm-project\build\bin\filecheck.exe D:\a\triton\triton\llvm-project\mlir\test\python\ir\auto_location.py
# executed command: C:/hostedtoolcache/windows/Python/3.11.9/x64/python3.exe 'D:\a\triton\triton\llvm-project\mlir\test\python\ir\auto_location.py'
# executed command: 'd:\a\triton\triton\llvm-project\build\bin\filecheck.exe' 'D:\a\triton\triton\llvm-project\mlir\test\python\ir\auto_location.py'
# .---command stderr------------
# | D:\a\triton\triton\llvm-project\mlir\test\python\ir\auto_location.py:37:11: error: CHECK: expected string not found in input
# | # CHECK: loc(callsite("testInferLocations"("{{.*}}[[SEP:[/\\]]]test[[SEP]]python[[SEP]]ir[[SEP]]auto_location.py":31:13 to :43) at callsite("run"("{{.*}}[[SEP]]test[[SEP]]python[[SEP]]ir[[SEP]]auto_location.py":13:4 to :7) at "<module>"("{{.*}}[[SEP]]test[[SEP]]python[[SEP]]ir[[SEP]]auto_location.py":26:1 to :4))))
# | ^
# | <stdin>:2:25: note: scanning from here
# | TEST: testInferLocations
# | ^
# |
# | Input file: <stdin>
# | Check file: D:\a\triton\triton\llvm-project\mlir\test\python\ir\auto_location.py
# |
# | -dump-input=help explains the following input dump.
# |
# | Input was:
# | <<<<<<
# | 1:
# | 2: TEST: testInferLocations
# | check:37 X error: no match found
# | 3: loc(callsite("testInferLocations"("D:\\a\\triton\\triton\\llvm-project\\mlir\\test\\python\\ir\\auto_location.py":31:13 to :43) at callsite("run"("D:\\a\\triton\\triton\\llvm-project\\mlir\\test\\python\\ir\\auto_location.py":13:4 to :7) at "<module>"("D:\\a\\triton\\triton\\llvm-project\\mlir\\test\\python\\ir\\auto_location.py":26:1 to :4))))
# | check:37 Probably this line should be changed [[SEP:[/\\]]] -> [[SEP]]?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Um no [[SEP:[/\\]]] defines SEP as the regex [/\\] so that's correct so I'm not sure what's going on there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fast response! I'll try to debug it further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That pattern should work and we do have windows tests here but actually I think they're not running because the CI (here, on Windows) doesn't have the minimum version of Python. I didn't have a Windows box handy when I put this together so it's true I wasn't able to test this thoroughly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I managed to fix it with the following regexp: [[SEP:(/|\\\\)]]
In the previous approach, only one \ character was written to SEP.
I checked it with lit and the following file:
// RUN: cat %s | FileCheck %s
# CHECK: loc(callsite("testInferLocations"("{{.*}}[[SEP:(/|\\\\)]]test[[SEP]]python[[SEP]]ir[[SEP]]auto_location.py":31:13 to :43) at callsite("run"("{{.*}}[[SEP]]test[[SEP]]python[[SEP]]ir[[SEP]]auto_location.py":13:4 to :7) at "<module>"("{{.*}}[[SEP]]test[[SEP]]python[[SEP]]ir[[SEP]]auto_location.py":26:1 to :4))))
1:
2: TEST: testInferLocations
3: loc(callsite("testInferLocations"("D:\\a\\triton\\triton\\llvm-project\\mlir\\test\\python\\ir\\auto_location.py":31:13 to :43) at callsite("run"("D:\\a\\triton\\triton\\llvm-project\\mlir\\test\\python\\ir\\auto_location.py":13:4 to :7) at "<module>"("D:\\a\\triton\\triton\\llvm-project\\mlir\\test\\python\\ir\\auto_location.py":26:1 to :4))))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok that makes sense but just for sanity check can you try SEP:[/\\]+
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But btw if you send a PR I'll happily approve it (otherwise I can send one soon).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SEP:[/\]+
yes, it also works. Do you prefer this way?
But btw if you send a PR I'll happily approve it (otherwise I can send one soon).
I'll open a PR for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially found in: #151246 (comment) To fix: ```txt ******************** TEST 'MLIR :: python/ir/auto_location.py' FAILED ******************** Exit Code: 1 Command Output (stdout): -- # RUN: at line 1 "C:/hostedtoolcache/windows/Python/3.11.9/x64/python3.exe" D:\a\triton\triton\llvm-project\mlir\test\python\ir\auto_location.py | d:\a\triton\triton\llvm-project\build\bin\filecheck.exe D:\a\triton\triton\llvm-project\mlir\test\python\ir\auto_location.py # executed command: C:/hostedtoolcache/windows/Python/3.11.9/x64/python3.exe 'D:\a\triton\triton\llvm-project\mlir\test\python\ir\auto_location.py' # executed command: 'd:\a\triton\triton\llvm-project\build\bin\filecheck.exe' 'D:\a\triton\triton\llvm-project\mlir\test\python\ir\auto_location.py' # .---command stderr------------ # | D:\a\triton\triton\llvm-project\mlir\test\python\ir\auto_location.py:37:11: error: CHECK: expected string not found in input # | # CHECK: loc(callsite("testInferLocations"("{{.*}}[[SEP:[/\\]]]test[[SEP]]python[[SEP]]ir[[SEP]]auto_location.py":31:13 to :43) at callsite("run"("{{.*}}[[SEP]]test[[SEP]]python[[SEP]]ir[[SEP]]auto_location.py":13:4 to :7) at "<module>"("{{.*}}[[SEP]]test[[SEP]]python[[SEP]]ir[[SEP]]auto_location.py":26:1 to :4)))) # | ^ # | <stdin>:2:25: note: scanning from here # | TEST: testInferLocations # | ^ # | # | Input file: <stdin> # | Check file: D:\a\triton\triton\llvm-project\mlir\test\python\ir\auto_location.py # | # | -dump-input=help explains the following input dump. # | # | Input was: # | <<<<<< # | 1: # | 2: TEST: testInferLocations # | check:37 X error: no match found # | 3: loc(callsite("testInferLocations"("D:\\a\\triton\\triton\\llvm-project\\mlir\\test\\python\\ir\\auto_location.py":31:13 to :43) at callsite("run"("D:\\a\\triton\\triton\\llvm-project\\mlir\\test\\python\\ir\\auto_location.py":13:4 to :7) at "<module>"("D:\\a\\triton\\triton\\llvm-project\\mlir\\test\\python\\ir\\auto_location.py":26:1 to :4)))) # | check:37 ```
Initially found in: llvm/llvm-project#151246 (comment) To fix: ```txt ******************** TEST 'MLIR :: python/ir/auto_location.py' FAILED ******************** Exit Code: 1 Command Output (stdout): -- # RUN: at line 1 "C:/hostedtoolcache/windows/Python/3.11.9/x64/python3.exe" D:\a\triton\triton\llvm-project\mlir\test\python\ir\auto_location.py | d:\a\triton\triton\llvm-project\build\bin\filecheck.exe D:\a\triton\triton\llvm-project\mlir\test\python\ir\auto_location.py # executed command: C:/hostedtoolcache/windows/Python/3.11.9/x64/python3.exe 'D:\a\triton\triton\llvm-project\mlir\test\python\ir\auto_location.py' # executed command: 'd:\a\triton\triton\llvm-project\build\bin\filecheck.exe' 'D:\a\triton\triton\llvm-project\mlir\test\python\ir\auto_location.py' # .---command stderr------------ # | D:\a\triton\triton\llvm-project\mlir\test\python\ir\auto_location.py:37:11: error: CHECK: expected string not found in input # | # CHECK: loc(callsite("testInferLocations"("{{.*}}[[SEP:[/\\]]]test[[SEP]]python[[SEP]]ir[[SEP]]auto_location.py":31:13 to :43) at callsite("run"("{{.*}}[[SEP]]test[[SEP]]python[[SEP]]ir[[SEP]]auto_location.py":13:4 to :7) at "<module>"("{{.*}}[[SEP]]test[[SEP]]python[[SEP]]ir[[SEP]]auto_location.py":26:1 to :4)))) # | ^ # | <stdin>:2:25: note: scanning from here # | TEST: testInferLocations # | ^ # | # | Input file: <stdin> # | Check file: D:\a\triton\triton\llvm-project\mlir\test\python\ir\auto_location.py # | # | -dump-input=help explains the following input dump. # | # | Input was: # | <<<<<< # | 1: # | 2: TEST: testInferLocations # | check:37 X error: no match found # | 3: loc(callsite("testInferLocations"("D:\\a\\triton\\triton\\llvm-project\\mlir\\test\\python\\ir\\auto_location.py":31:13 to :43) at callsite("run"("D:\\a\\triton\\triton\\llvm-project\\mlir\\test\\python\\ir\\auto_location.py":13:4 to :7) at "<module>"("D:\\a\\triton\\triton\\llvm-project\\mlir\\test\\python\\ir\\auto_location.py":26:1 to :4)))) # | check:37 ```
**Context:** Update llvm, stablehlo and enzyme, 2025 Q4 The latest pair of good versions, indicated by stablehlo, is openxla/stablehlo@0a4440a ``` stablehlo=0a4440a5c8de45c4f9649bf3eb4913bf3f97da0d llvm=113f01aa82d055410f22a9d03b3468fa68600589 ``` For Enzyme, we go to the latest release https://github.com/EnzymeAD/Enzyme/releases/tag/v0.0.203 ``` enzyme=v0.0.203 ``` with commit `476c8e3193a8577ba24ff845ae2294109225f83a` **Description of the Change:** - `llvm-project/mlir/include/mlir/InitAllPasses.h` header no longer includes individual pass headers. To incorporate specific passes, you now need to include either `mlir/Conversion/Passes.h` or the header for each dialect pass. This change stems from the upstream cleanup of `mlir/InitAllDialects.h`, which previously handled these includes. Following the decision from the owner (as seen in this pull request comment: https://github.com/ftynse/water/pull/16/files#r2259526343), we will specifically include each required header, faster compilation (no unnecessary headers). For further context, refer to this PR: llvm/llvm-project#151150 - To bufferize custom tensors into custom buffers. The upstream PR llvm/llvm-project#144867 changed the the return type of `bufferization::detail::defaultGetBufferType` to be `BufferLikeType` instead of `BaseMemRefType`. We aligned the behaviour with the upstream PR, return the BufferLikeType from our `getBufferType()` implementation as well. - `elemwise_unary` and `elemwise_binary` of `linalg` have been deprecated from the upstream, replaced with `elementwise`. llvm/llvm-project#147082. Follow this PR, using `linalg.add` directly. register_all_dialects/passes/extension` should now be configured as a static library in CMake, refer to this PR: llvm/llvm-project#151150 - An opt-in feature (`{op}::create` method) be added to support the same behaviour of `rewriter.create` method. But it provides a meaningful message when using this new opt-in feature compared to `rewrite.create`. Since the new generated `create` function will call the `build` function, it requires us to define the instance of the builder (`GraidentOps.td`). Refer to the PR: llvm/llvm-project#147168. **Take `linalg.abs` as an example:** ```cpp void AbsOp::build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ValueRange inputs, ValueRange outputs, ArrayRef<NamedAttribute> attributes) { buildStructuredOp(odsBuilder, odsState, std::nullopt, inputs, outputs, attributes, AbsOp::getRegionBuilder()); } AbsOp AbsOp::create(::mlir::OpBuilder &builder, ::mlir::Location location, ValueRange inputs, ValueRange outputs, ArrayRef<NamedAttribute> attributes) { ::mlir::OperationState __state__(location, getOperationName()); build(builder, __state__, inputs, outputs, attributes); auto __res__ = ::llvm::dyn_cast<AbsOp>(builder.create(__state__)); assert(__res__ && "builder didn't return the right type"); return __res__; } AbsOp AbsOp::create(::mlir::ImplicitLocOpBuilder &builder, ValueRange inputs, ValueRange outputs, ArrayRef<NamedAttribute> attributes) { return create(builder, builder.getLoc(), inputs, outputs, attributes); } ``` - Changed `EnzymeStatic-21` to `EnzymeStatic-22` - Mock `_ods_cext.globals.register_traceback_file_exclusion` due to API conflicts between Catalyst's MLIR version and the MLIR version used by JAX. The current JAX version we used has not yet updated to the latest MLIR, causing compatibility issues. This workaround will be removed once JAX updates to a compatible MLIR version. llvm/llvm-project#151246 TODO: - [x] Update .dep_version - [ ] Change all `rewrite.create` to `{op}::create` Even it’s compatible to the current `rewrite.create` method, we don’t need to change all `rewrite.create` to `{op}::create` right away, but this new feature provides more friendly development experience. Just for an example: llvm/llvm-project#147311 **Benefits:** **Possible Drawbacks:** **Related GitHub Issues:** [sc-100911] [sc-100912] --------- Co-authored-by: David Ittah <[email protected]>
This PR implements "automatic" location inference in the bindings. The way it works is it walks the frame stack collecting source locations (Python captures these in the frame itself). It is adapted from JAX's implementation but moves the frame stack traversal into the bindings for better performance.
The system supports registering "included" and "excluded" filenames; frames originating from functions in included filenames will not be filtered and frames originating from functions in excluded filenames will be filtered (in that order). This allows excluding all the generated
*_ops_gen.pyfiles.The system is also "toggleable" and off by default to save people who have their own systems (such as JAX) from the added cost.
Note, the system stores the entire stacktrace (subject to
locTracebackFramesLimit) in theLocationusing specifically aCallSiteLoc. This can be useful for profiling tools (flamegraphs etc.).Shoutout to the folks at JAX for coming up with a good system (cc @hawkinsp 😄).