Skip to content

Update llvm, 2025 Q3 #1916

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

Open
wants to merge 38 commits into
base: main
Choose a base branch
from
Open

Update llvm, 2025 Q3 #1916

wants to merge 38 commits into from

Conversation

paul0403
Copy link
Member

@paul0403 paul0403 commented Jul 17, 2025

Context:
Update llvm, mhlo and enzyme, 2025 Q3.
The latest pair of good versions, indicated by mhlo, is tensorflow/mlir-hlo@1dd2e71

mhlo=1dd2e71331014ae0373f6bf900ce6be393357190
llvm=f8cb7987c64dcffb72414a40560055cb717dbf74

For Enzyme, we go to the latest release https://github.com/EnzymeAD/Enzyme/releases/tag/v0.0.186

enzyme=v0.0.186

with commit 8c1a596158f6194f10e8ffd56a1660a61c54337e

Description of the Change:
Miscellaneous:

  1. GreedyRewriteConfig.stuff = blah -> GreedyRewriteConfig.setStuff(blah) [mlir] add a fluent API to GreedyRewriterConfig llvm/llvm-project#137122
  2. llvm gep op inbounds attribute is subsumed under a gep sign wrap enum flag [mlir][llvm] Support nusw and nuw in GEP llvm/llvm-project#137272
  3. arith::Constant[Int, Float]Op builders now have the same argument order as other ops (output type first, then arguments) switch type and value ordering for arith Constant[XX]Op llvm/llvm-project#144636 (note that Enzyme also noticed this Update to newer llvm EnzymeAD/Enzyme#2379 😆 )
  4. The lookupOrCreateFn functions now take in a builder instead of instantiating a new one [mlir][LLVM] Add OpBuilder & to lookupOrCreateFn functions llvm/llvm-project#136421
  5. getStridedElementPtr now takes in rewriter as the first argument (instead of the last), like all the other utils emit inbounds and nuw attributes in memref. llvm/llvm-project#138984
  6. The following functions now return a LogicalResult, and will be caught by warnings as errors as -Wunused-result:

Things related to transform.apply_registered_pass op:

  1. It now takes in a dynamic_options [MLIR][Transform] Allow ApplyRegisteredPassOp to take options as a param llvm/llvm-project#142683. We don't need to use this as all our pass options are static.
  2. The options it takes in are now dictionaries instead of strings [MLIR][Transform] apply_registered_pass op's options as a dict llvm/llvm-project#143159

Bufferization:

  1. bufferization.to_memref op is renamed to bufferization.to_buffer [mlir][bufferization][NFC] Rename to_memref to to_buffer llvm/llvm-project#137180
  2. bufferization.to_tensor op's builder now needs the result type to be explicit [mlir][bufferization] Support custom types (1/N) llvm/llvm-project#142986. This is also needed by a patched mhlo pass.
  3. The getBuffer() methods take in a new arg for BufferizationState [MLIR] Add bufferization state class to OneShotBufferization pass llvm/llvm-project#141019, [MLIR] Add bufferization state to getBufferType and resolveConflicts interface methods llvm/llvm-project#141466
  4. UnknownTypeConverterFn in bufferization options now takes in just a type instead of a full value [mlir][bufferization] Use Type instead of Value in unknown conversion llvm/llvm-project#144658

[sc-95176]

@paul0403 paul0403 marked this pull request as ready for review July 17, 2025 20:55
@paul0403

This comment was marked as outdated.

@@ -35,7 +35,7 @@ static Value genSelectiveShift(PatternRewriter &rewriter, Location loc, Value pa
}

// Make sure all active iteration variables match the selectors.
Value shiftCondition = rewriter.create<arith::ConstantIntOp>(loc, true, 1);
Value shiftCondition = rewriter.create<arith::ConstantIntOp>(loc, 1, true);
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: there are 3 arith::ConstantInt buidler calls in the mhlo patched passes. We might need to update those too.

@paul0403

This comment was marked as outdated.

@@ -97,6 +97,11 @@ mhlo:
@if cd mlir-hlo; git apply --check $(MK_DIR)/patches/mhlo-add-back-necessary-passes.patch; then \
git apply $(MK_DIR)/patches/mhlo-add-back-necessary-passes.patch; \
fi

# Patch a MHLO bug with std::sort
Copy link
Member Author

@paul0403 paul0403 Jul 22, 2025

Choose a reason for hiding this comment

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

Remember to patch this in github wheels scripts too

Comment on lines +18 to +19
- sort(r->nodes, &r->deltab);
- sort(r->nodes, &r->deltaf);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is equivalent:

diff --git a/utils/cycle_detector.cc b/utils/cycle_detector.cc
index e3901ae88..890f39654 100644
--- a/utils/cycle_detector.cc
+++ b/utils/cycle_detector.cc
@@ -199,8 +199,8 @@ static void backwardDfs(GraphCycles::Rep* r, int32_t n, int32_t lowerBound) {
 // Recomputes rank assignments to make them compatible with the edges (producer
 // has smaller rank than its consumer)
 static void reorder(GraphCycles::Rep* r) {
-  sort(r->nodes, &r->deltab);
-  sort(r->nodes, &r->deltaf);
+  mlir::sort(r->nodes, &r->deltab);
+  mlir::sort(r->nodes, &r->deltaf);
 
   // Adds contents of delta lists to list (backwards deltas first).
   r->list.clear();

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is the same kind of fix as just renaming? as in explicitly forcing the name?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes.

As a QoL, also add util to convert python values into mlir attributes
@paul0403 paul0403 requested review from a team, mehrdad2m and dime10 July 23, 2025 19:01
@erick-xanadu
Copy link
Contributor

It now takes in a dynamic_options
llvm/llvm-project#142683. We don't need to use this as all our pass options are static.
The options it takes in are now dictionaries instead of strings
llvm/llvm-project#143159

Thank you for pointing this out. I'll try to address these tomorrow in xDSL.

@paul0403
Copy link
Member Author

It now takes in a dynamic_options
llvm/llvm-project#142683. We don't need to use this as all our pass options are static.
The options it takes in are now dictionaries instead of strings
llvm/llvm-project#143159

Thank you for pointing this out. I'll try to address these tomorrow in xDSL.

cc @mehrdad2m

@paul0403
Copy link
Member Author

I think this PR in llvm llvm/llvm-project#138125 is segfaulting all the async tests.

@paul0403
Copy link
Member Author

I have isolated it and this is an upstream bug. I opened an issue in llvm llvm/llvm-project#150441

@erick-xanadu
Copy link
Contributor

erick-xanadu commented Jul 24, 2025

@mehrdad2m @paul0403 PR regarding the transform dialect in xDSL PennyLaneAI/pennylane#7956

@@ -165,3 +166,49 @@ def custom_lower_jaxpr_to_module(
worklist += [*op.body.operations]

return ctx.module, ctx.context


def get_mlir_attribute_from_pyval(value):
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a very nice utility function. Do you think it could belong to the utils folder or is it too specific to this file?

Copy link
Contributor

Choose a reason for hiding this comment

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

I have a file which contains similar code named types.py. You could also think of upstreaming something like this to the MLIR repo. (which will then be picked up by JAX since they use the Python bindings) Maybe somewhere around this file

Copy link
Member Author

Choose a reason for hiding this comment

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

This looks like a very nice utility function. Do you think it could belong to the utils folder or is it too specific to this file?

I think this is specific to this file, this is a helper just for lowering jax primitive params (which are pure python values) to mlir attributes, so I thought it naturally belonged in the jax_extras/lowering.py. This file already is the util file for lowering I think.

@@ -204,6 +219,11 @@ clean-dialects:
clean-llvm:
@echo "clean llvm/mlir build files"
rm -rf $(LLVM_BUILD_DIR)
cd llvm-project; git clean -fd; git checkout .
Copy link
Contributor

Choose a reason for hiding this comment

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

should we add the same logic for the other pathed repos (enzyme, mhlo) for consistency?

Copy link
Member Author

Choose a reason for hiding this comment

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

I already have this for mhlo. I will add this for enzyme as well.

Copy link

codecov bot commented Jul 26, 2025

Codecov Report

❌ Patch coverage is 86.11111% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.77%. Comparing base (14ef3a6) to head (eb82dc4).

Files with missing lines Patch % Lines
frontend/catalyst/jax_extras/lowering.py 82.75% 3 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1916      +/-   ##
==========================================
- Coverage   96.81%   96.77%   -0.05%     
==========================================
  Files          87       87              
  Lines        9682     9715      +33     
  Branches      899      911      +12     
==========================================
+ Hits         9374     9402      +28     
- Misses        245      248       +3     
- Partials       63       65       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants