Skip to content

Allow caller of classify_argument to specify that the first arg is sret #272

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vchuravy
Copy link
Member

classify_arguments uses the function type so it can't do that
determination on it's own.

cc: @wsmoses

`classify_arguments` uses the function type so it can't do that
determination on it's own.
@wsmoses
Copy link
Contributor

wsmoses commented Dec 17, 2021

Also sret as an attribute is not always set even if Julia makes an sret.

E.g. the code snippet in EnzymeAD/Enzyme.jl#154 will result in the first arg as sret, but without the attribute.

@vchuravy
Copy link
Member Author

julia> function meta(a, out, cond)
           if cond
               out[] = a
           end
       end
meta (generic function with 1 method)

julia> @code_llvm meta(2.0, Ref(0.0), true)
;  @ REPL[1]:1 within `meta`
define { {}*, i8 } @julia_meta_141([8 x i8]* noalias nocapture align 8 dereferenceable(8) %0, double %1, {}* nonnull align 8 dereferenceable(8) %2, i8 zeroext %3) #0 {
top:
;  @ REPL[1]:2 within `meta`
  %4 = and i8 %3, 1
  %.not = icmp eq i8 %4, 0
  br i1 %.not, label %L4, label %L2

L2:                                               ; preds = %top
;  @ REPL[1]:3 within `meta`
; ┌ @ refvalue.jl:57 within `setindex!`
; │┌ @ Base.jl:43 within `setproperty!`
    %5 = bitcast {}* %2 to double*
    store double %1, double* %5, align 8
; └└
  %.0..sroa_cast = bitcast [8 x i8]* %0 to double*
  store double %1, double* %.0..sroa_cast, align 8
  br label %L4

L4:                                               ; preds = %L2, %top
  %merge = phi { {}*, i8 } [ { {}* null, i8 1 }, %top ], [ { {}* null, i8 2 }, %L2 ]
  ret { {}*, i8 } %merge
}

@codecov
Copy link

codecov bot commented Dec 17, 2021

Codecov Report

Merging #272 (f6985ee) into master (3e9b441) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #272   +/-   ##
=======================================
  Coverage   87.12%   87.12%           
=======================================
  Files          22       22           
  Lines        2112     2112           
=======================================
  Hits         1840     1840           
  Misses        272      272           
Impacted Files Coverage Δ
src/irgen.jl 94.39% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e9b441...f6985ee. Read the comment docs.

@maleadt
Copy link
Member

maleadt commented Dec 17, 2021

We could pass it the function instead, but I guess that first need a fix in Julia to set the sret attribute.

@wsmoses
Copy link
Contributor

wsmoses commented Dec 17, 2021

Only relevant for code calling the function in this PR, I remember @vtjnash said there was a compile-time performance reason why sret wasn't set and it didn't matter for correctness so I'm not sure there will be a proper Julia fix so much as a "derive it in the same way".

@maleadt
Copy link
Member

maleadt commented Dec 17, 2021

Can we easily derive it? Otherwise that'd be a great thing for classify_arguments to do.

@vtjnash
Copy link
Contributor

vtjnash commented Dec 17, 2021

There are also sometimes 2 sret arguments. In codegen we call struct jl_returninfo_t get_specsig_function() to re-compute this information precisely.

@vchuravy vchuravy marked this pull request as draft March 4, 2022 16:17
@maleadt maleadt force-pushed the master branch 5 times, most recently from 1d233d7 to e18b7c2 Compare January 20, 2025 10:33
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants