Skip to content

[Metal] sincos parameter attributes#762

Merged
maleadt merged 3 commits into
mainfrom
sincos_intr
Jun 5, 2026
Merged

[Metal] sincos parameter attributes#762
maleadt merged 3 commits into
mainfrom
sincos_intr

Conversation

@christiangnrd
Copy link
Copy Markdown
Member

@christiangnrd christiangnrd commented Jan 27, 2026

Close #663. This does NOT resolve issue #761 but I wrote it while troubleshooting it and figured I may as well open a PR.

@github-actions
Copy link
Copy Markdown
Contributor

Your PR requires formatting changes to meet the project's style guidelines.
Please consider running Runic (git runic master) to apply these changes.

Click here to view the suggested changes.
diff --git a/src/metal.jl b/src/metal.jl
index fd571f1..5142774 100644
--- a/src/metal.jl
+++ b/src/metal.jl
@@ -1013,7 +1013,7 @@ function annotate_air_intrinsics!(@nospecialize(job::CompilerJob), mod::LLVM.Mod
                 end
                 push!(fn_attrs, EnumAttribute(name, 0))
             end
-            changed = true
+            return changed = true
         end
 
         function add_param_attributes(idx, names...)
@@ -1028,7 +1028,7 @@ function annotate_air_intrinsics!(@nospecialize(job::CompilerJob), mod::LLVM.Mod
         if fn == "air.wg.barrier" || fn == "air.simdgroup.barrier"
             add_fn_attributes("nounwind", "mustprogress", "convergent", "willreturn")
 
-        # sincos
+            # sincos
         elseif match(r"^air.sincos", fn) !== nothing
             add_param_attributes(2, "nocapture", "writeonly")
 

@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 27, 2026

Codecov Report

❌ Patch coverage is 45.45455% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.46%. Comparing base (a912d8e) to head (5f3511f).

Files with missing lines Patch % Lines
src/metal.jl 45.45% 12 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #762      +/-   ##
==========================================
+ Coverage   79.41%   79.46%   +0.04%     
==========================================
  Files          25       25              
  Lines        4591     4601      +10     
==========================================
+ Hits         3646     3656      +10     
  Misses        945      945              

☔ View full report in Codecov by Harness.
📢 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.

@maleadt
Copy link
Copy Markdown
Member

maleadt commented Jan 27, 2026

Are we correctly emitting sincos with ptr arguments from Metal.jl? Otherwise the attribute won't help.

Copy link
Copy Markdown
Member

@maleadt maleadt left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

It would be good to start adding tests for things like this though. Should be reasonable with the FileCheck infrastructure we have nowadays.

@christiangnrd
Copy link
Copy Markdown
Member Author

I agree that tests are probably a good idea. I'll look into FileCheck at some point in the near future. Merging can probably wait until I get around to tests (unless someone speaks up otherwise).

- extend the regex to also match `air.fast_sincos.*`, which Metal.jl
  emits for `FastMath.sincos_fast` with the same signature
- emit `captures(none)` instead of `nocapture` on LLVM 21+, matching
  the existing workarounds elsewhere
- add a FileCheck test covering the annotated declarations

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@maleadt maleadt merged commit 0632a9c into main Jun 5, 2026
37 checks passed
@maleadt maleadt deleted the sincos_intr branch June 5, 2026 16:32
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.

[Metal] Add sincos intrinsic LLVM annotations

2 participants