Skip to content

Conversation

@wsmoses
Copy link
Member

@wsmoses wsmoses commented Sep 15, 2025

No description provided.

@codecov
Copy link

codecov bot commented Sep 16, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.75%. Comparing base (8040a3e) to head (99fbaeb).
⚠️ Report is 13 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1666      +/-   ##
==========================================
- Coverage   68.82%   67.75%   -1.07%     
==========================================
  Files         103      108       +5     
  Lines       11596    11783     +187     
==========================================
+ Hits         7981     7984       +3     
- Misses       3615     3799     +184     

☔ 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.

if attr isa Nothing
return false
end
for i in 1:length(attr)
Copy link
Member

Choose a reason for hiding this comment

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

I'm on the phone, but I think this entire for loop until the final return can be written as

any(at -> String(at) == "write", attr)

Copy link
Member Author

Choose a reason for hiding this comment

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

problem is that attr doesn't define an iterator

Copy link
Member

Choose a reason for hiding this comment

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

It has indices but isn't iterable? That sounds unfortunate, the basic iteration interface is a single function: https://docs.julialang.org/en/v1/manual/interfaces/#man-interface-iteration

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah I mean ironically the definition of that is in this repo (attribute.jl)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can push this if you want?

diff --git a/src/TracedUtils.jl b/src/TracedUtils.jl
index 64619741..7d6d4de1 100644
--- a/src/TracedUtils.jl
+++ b/src/TracedUtils.jl
@@ -278,12 +278,7 @@ function is_pure(func)
     if attr isa Nothing
         return false
     end
-    for i in 1:length(attr)
-        sa = Base.String(attr[i])
-        if sa == "write"
-            return false
-        end
-    end
+    any(at -> String(at) == "write", attr) && return false
     return true
 end
 
diff --git a/src/mlir/IR/Attribute.jl b/src/mlir/IR/Attribute.jl
index 1321c3e9..ec12785d 100644
--- a/src/mlir/IR/Attribute.jl
+++ b/src/mlir/IR/Attribute.jl
@@ -802,6 +802,14 @@ function Base.getindex(attr::Attribute, i)
     end
 end
 
+function Base.iterate(attr::Attribute, state=1)
+    if state > length(attr)
+        nothing
+    else
+        (attr[state], state + 1)
+    end
+end
+
 function Base.getindex(attr::Attribute)
     @assert isdenseelements(attr) "attribute $(attr) is not a dense elements attribute"
     @assert issplat(attr) "attribute $(attr) is not splatted (more than one different elements)"

Copy link
Member Author

Choose a reason for hiding this comment

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

go for it!

wsmoses and others added 2 commits September 15, 2025 19:17
Co-authored-by: Mosè Giordano <765740+giordano@users.noreply.github.com>
result_shardings,
mlir_fn_res.global_device_ids,
donated_args_mask,
Reactant.TracedUtils.is_pure(func3)
Copy link
Contributor

Choose a reason for hiding this comment

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

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
Reactant.TracedUtils.is_pure(func3)
Reactant.TracedUtils.is_pure(func3),

- `is_pure`: Whether the function being compiled is pure (e.g. has a side effect, like MPI.Send)
"""
function codegen_xla_call(flatten_names, nresults, is_sharded::Bool, ndevices::Int)
function codegen_xla_call(flatten_names, nresults, is_sharded::Bool, ndevices::Int, is_pure::Bool)
Copy link
Contributor

Choose a reason for hiding this comment

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

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
function codegen_xla_call(flatten_names, nresults, is_sharded::Bool, ndevices::Int, is_pure::Bool)
function codegen_xla_call(
flatten_names, nresults, is_sharded::Bool, ndevices::Int, is_pure::Bool
)


concretized_res_names, xla_call_code = codegen_xla_call(
flatten_arg_names, length(linear_results), mlir_fn_res.is_sharded, ndevices
flatten_arg_names, length(linear_results), mlir_fn_res.is_sharded, ndevices, mlir_fn_res.is_pure
Copy link
Contributor

Choose a reason for hiding this comment

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

[JuliaFormatter] reported by reviewdog 🐶

Suggested change
flatten_arg_names, length(linear_results), mlir_fn_res.is_sharded, ndevices, mlir_fn_res.is_pure
flatten_arg_names,
length(linear_results),
mlir_fn_res.is_sharded,
ndevices,
mlir_fn_res.is_pure,

missing,
global_device_ids,
nothing, # populated later in `compile_mlir!`
is_pure(func2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor: but not necessary to actually calculate this here since it's recalculated in compile_mlir! I think. Could just pass false instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

the mlir func may go out of scope so we do need to precompute

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh I see

@romanlee
Copy link
Collaborator

Looks good from my side besides the comments above

wsmoses and others added 2 commits September 16, 2025 19:42
Co-authored-by: Roman Lee <31547765+romanlee@users.noreply.github.com>
@wsmoses wsmoses merged commit 410ee6e into main Sep 17, 2025
68 of 74 checks passed
@wsmoses wsmoses deleted the memeffects branch September 17, 2025 18:27
@giordano
Copy link
Member

@wsmoses
Copy link
Member Author

wsmoses commented Sep 17, 2025

oh let me fix that in post, I thought docs failure was expected from some ssl issue but thats clearly not

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