Implement always_inline as a CompilerJob property

[vchuravy]

@maleadt remind me should we put always_inline into runtime_slug?

I also noticed that PTXCompilerTarget implements hash, but GCNCompilerTarget doesn't.

[maleadt]

@maleadt remind me should we put always_inline into runtime_slug?

In principle yes, although in practice the runtime probably won't contain functions with non-inlined calls.

[codecov]

Codecov Report

Base: 73.79% // Head: 63.67% // Decreases project coverage by -10.11% ⚠️

Coverage data is based on head (aa18542) compared to base (9f3186b).
Patch coverage: 57.14% of modified lines in pull request are covered.

❗ Current head aa18542 differs from pull request most recent head f3e03a4. Consider uploading reports for the commit f3e03a4 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           master     #373       +/-   ##
===========================================
- Coverage   73.79%   63.67%   -10.12%     
===========================================
  Files          24       24               
  Lines        2816     2676      -140     
===========================================
- Hits         2078     1704      -374     
- Misses        738      972      +234     

Impacted Files	Coverage Δ
src/bpf.jl	0.00% <0.00%> (-92.86%)	⬇️
src/gcn.jl	0.00% <0.00%> (-67.86%)	⬇️
src/metal.jl	0.00% <0.00%> (ø)
src/interface.jl	84.31% <100.00%> (-2.82%)	⬇️
src/native.jl	81.25% <100.00%> (-12.09%)	⬇️
src/ptx.jl	89.40% <100.00%> (-3.82%)	⬇️
src/spirv.jl	87.41% <100.00%> (-3.26%)	⬇️
src/validation.jl	44.20% <0.00%> (-53.04%)	⬇️
... and 19 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[jpsamaroo]

@vchuravy this seems to be working well for @luraess when enabled in AMDGPU. 👍 from me.

[maleadt]

I'm not a fan of adding common properties to each back-end's Target struct, which up until now only contains target-specific elements. Wouldn't it be better to make this a property of the CompilerJob?

[jpsamaroo]

Is this ready to merge? This has been working excellently for AMDGPU since the latest push.

[maleadt]

GCN tests are failing because the expensive function always seems inlined. @jpsamaroo care to take a look?

[jpsamaroo]

It's not actually inlined (I have s_add_u32 s20, s20, julia_f_expensive_3623@rel32@lo+4, which is leading up to an indirect jump), but the ASM for the called function isn't shown in the output.

[jpsamaroo]

Using dump_module=true causes a segfault for me in the AMDGPU instruction printer, so I'm going to just parse the LLVM IR.

[maleadt]

Using dump_module=true causes a segfault for me in the AMDGPU instruction printer, so I'm going to just parse the LLVM IR.

Doesn't that indicate a deeper problem?

[jpsamaroo]

@maleadt is it intended that always_inline was removed from PTXCompilerTarget?

[jpsamaroo]

Doesn't that indicate a deeper problem?

Maybe, but the AMDGPU target is in frequent flux, so I wouldn't be surprised if it works fine on LLVM 15/16.

[maleadt]

Yes, I suggested above putting it in the compiler job instead, as it isn't really a property of the target, is it? Does this cause any issues maybe?

[jpsamaroo]

Right, so I guess the fact that CUDA.jl is trying to pass it through the target just needs to be fixed. Makes sense!