Skip to content
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

remove let from @setup_workload #16

Merged
merged 4 commits into from
May 8, 2023
Merged

remove let from @setup_workload #16

merged 4 commits into from
May 8, 2023

Conversation

KristofferC
Copy link
Sponsor Member

This seems to have a bad effect on precompilation somehow.

I investigated this from JuliaSIMD/LoopVectorization.jl#490. Removing these let statements makes the regression go away.

@KristofferC
Copy link
Sponsor Member Author

I don't really understand the test error on nightly.

@timholy
Copy link
Sponsor Member

timholy commented May 8, 2023

This seems to have a bad effect on precompilation somehow.

I think it's because the let must trigger inference and/or codegen before reaching the @compile_workload. If runtime-dispatched callees get compiled outside the "scope" of the snooping that it turns on, they are not guaranteed to be cached. (They might be, by the ordinary rules, but it's not guaranteed.)

I don't really understand the test error on nightly.

Nightly infers both a specialized and an unspecialized version without the let. The let is presumably helping inference avoid the need for the unspecialized version. Not sure what change made it specific to nightly, though.

@codecov
Copy link

codecov bot commented May 8, 2023

Codecov Report

Merging #16 (90b1597) into main (302eeae) will decrease coverage by 0.08%.
The diff coverage is 100.00%.

❗ Current head 90b1597 differs from pull request most recent head 8dec582. Consider uploading reports for the commit 8dec582 to get more accurate results

@@            Coverage Diff             @@
##             main      #16      +/-   ##
==========================================
- Coverage   93.25%   93.18%   -0.08%     
==========================================
  Files           3        3              
  Lines          89       88       -1     
==========================================
- Hits           83       82       -1     
  Misses          6        6              
Impacted Files Coverage Δ
src/workloads.jl 87.75% <100.00%> (-0.25%) ⬇️

@timholy
Copy link
Sponsor Member

timholy commented May 8, 2023

Thanks for getting this out the door!

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.

2 participants