Skip to content

[Metal] sincos parameter attributes#762

Open
christiangnrd wants to merge 2 commits intomasterfrom
sincos_intr
Open

[Metal] sincos parameter attributes#762
christiangnrd wants to merge 2 commits intomasterfrom
sincos_intr

Conversation

@christiangnrd
Copy link
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
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

codecov bot commented Jan 27, 2026

Codecov Report

❌ Patch coverage is 21.05263% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.73%. Comparing base (4e5ffed) to head (66dffe8).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
src/metal.jl 21.05% 15 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #762      +/-   ##
==========================================
- Coverage   74.97%   74.73%   -0.24%     
==========================================
  Files          24       24              
  Lines        3764     3772       +8     
==========================================
- Hits         2822     2819       -3     
- Misses        942      953      +11     

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

@maleadt
Copy link
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
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
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).

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