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

Add hexagon implementations for compiler_rt builtins #22029

Merged
merged 8 commits into from
Nov 24, 2024

Conversation

androm3da
Copy link
Contributor

No description provided.

lib/compiler_rt/hexagon.zig Outdated Show resolved Hide resolved
@alexrp
Copy link
Member

alexrp commented Nov 20, 2024

I see these also referenced in LLVM:

  • __hexagon_floattidf
  • __hexagon_floattisf
  • __hexagon_fixunssfti
  • __hexagon_fixunsdfti
  • __hexagon_fixsfti
  • __hexagon_fixdfti
  • __hexagon_fast2_sqrtf
  • __hexagon_fast2_sqrtdf2
  • __hexagon_adddf3 and __hexagon_fast_adddf3
  • __hexagon_subdf3 and __hexagon_fast_subdf3
  • __hexagon_muldf3 and __hexagon_fast_muldf3
  • __hexagon_divdf3 and __hexagon_fast_divdf3
  • __hexagon_divsf3 and __hexagon_fast_divsf3

Do we need these?

@androm3da
Copy link
Contributor Author

I see these also referenced in LLVM:

  • __hexagon_floattidf
  • __hexagon_floattisf
  • __hexagon_fixunssfti
  • __hexagon_fixunsdfti
  • __hexagon_fixsfti
  • __hexagon_fixdfti

These are not defined in compiler-rt/lib/builtins/hexagon/ so I think they're safe to omit. But I'll follow up with the team to understand why these references aren't satisfied.

  • __hexagon_fast2_sqrtf
  • __hexagon_fast2_sqrtdf2
  • __hexagon_adddf3 and __hexagon_fast_adddf3
  • __hexagon_subdf3 and __hexagon_fast_subdf3
  • __hexagon_muldf3 and __hexagon_fast_muldf3
  • __hexagon_divdf3 and __hexagon_fast_divdf3
  • __hexagon_divsf3 and __hexagon_fast_divsf3

Do we need these?

These were unintentionally omitted. I'll see about revising this PR to include these.

I assume it's safe/permitted/expected for me to use .set directives to create aliases from within an inline assembly block? Otherwise I will need to find another mechanism to publish the aliases.

@alexrp
Copy link
Member

alexrp commented Nov 20, 2024

I assume it's safe/permitted/expected for me to use .set directives to create aliases from within an inline assembly block? Otherwise I will need to find another mechanism to publish the aliases.

Can you clarify what aliases you're referring to?

@androm3da
Copy link
Contributor Author

Can you clarify what aliases you're referring to?

The __hexagon_fast_* / __qdsp_* / __hexagon_fast2_* aliases, for example:

https://github.com/llvm/llvm-project/blob/e14827f0828d14ef17ab76316e8449d1b76e2617/compiler-rt/lib/builtins/hexagon/dfaddsub.S#L58-L71

https://github.com/rust-lang/compiler-builtins/blob/a09218f1c4d23ffbd97d68f0fefb5feed2469dc5/src/hexagon/dfaddsub.s#L7-L15

@alexrp
Copy link
Member

alexrp commented Nov 20, 2024

Hmm, I'm not sure if doing that in inline assembly is a good idea. 🤔 But if these are just aliases of the same symbol, perhaps you can just do multiple @export()s with different names?

@androm3da
Copy link
Contributor Author

Hmm, I'm not sure if doing that in inline assembly is a good idea. 🤔 But if these are just aliases of the same symbol, perhaps you can just do multiple @export()s with different names?

I thought such a thing might be possible, so yeah, that'll do. I will work on it.

lib/compiler_rt/hexagon.zig Outdated Show resolved Hide resolved
lib/compiler_rt/hexagon.zig Outdated Show resolved Hide resolved
lib/compiler_rt/hexagon.zig Outdated Show resolved Hide resolved
lib/compiler_rt/hexagon.zig Outdated Show resolved Hide resolved
lib/compiler_rt/hexagon.zig Outdated Show resolved Hide resolved
@androm3da androm3da force-pushed the bcain/hex_crt branch 2 times, most recently from 12d69ea to 782529d Compare November 22, 2024 14:26
androm3da and others added 7 commits November 22, 2024 15:14
Signed-off-by: Brian Cain <[email protected]>
I was under the mistaken assumption I needed a `call` here but an
extended jump will suffice.  The call on its own would have failed
without an allocframe/dealloc.

Signed-off-by: Brian Cain <[email protected]>
@androm3da
Copy link
Contributor Author

Addressed review comments, fixed __hexagon_subdf3 issue (thanks @SidManning)

@alexrp alexrp linked an issue Nov 22, 2024 that may be closed by this pull request
Copy link
Member

@alexrp alexrp left a comment

Choose a reason for hiding this comment

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

Small nitpick for consistency.

lib/compiler_rt/hexagon.zig Outdated Show resolved Hide resolved
lib/compiler_rt/hexagon.zig Outdated Show resolved Hide resolved
lib/compiler_rt/hexagon.zig Outdated Show resolved Hide resolved
lib/compiler_rt/hexagon.zig Outdated Show resolved Hide resolved
@alexrp
Copy link
Member

alexrp commented Nov 22, 2024

These are not defined in compiler-rt/lib/builtins/hexagon/ so I think they're safe to omit. But I'll follow up with the team to understand why these references aren't satisfied.

This looks like an LLVM bug to me. They seem to be used for i128 <-> f32/f64 conversions. Clang disables __int128 for Hexagon, so it's not an issue there, but it is an issue for Zig:

cat test.zig
export fn cvt(d: i128) f64 {
    return @floatFromInt(d);
}zig4 build-lib test.zig -fno-compiler-rt -target hexagon-freestandingllvm-objdump -r --disassemble-symbols=cvt libtest.a

libtest.a(./libtest.a.o):       file format elf32-hexagon

Disassembly of section .text:

00000000 <cvt>:
       0:       02 c0 9d a0     a09dc002 {      allocframe(#0x10) }
       4:       04 c2 03 f5     f503c204 {      r5:4 = combine(r3,r2) }
       8:       04 c0 01 f5     f501c004 {      r5:4 = combine(r1,r0) }
       c:       ff e2 de a7     a7dee2ff {      memd(r30+#-0x8) = r3:2 }
      10:       fe e0 de a7     a7dee0fe {      memd(r30+#-0x10) = r1:0 }
      14:       00 c0 00 5a     5a00c000 {      call 0x14 }                     00000014:  R_HEX_B22_PCREL      __hexagon_floattidf
      18:       1e c0 1e 96     961ec01e {      dealloc_return }

@alexrp
Copy link
Member

alexrp commented Nov 23, 2024

So, I think these calls should just all be deleted. That way, the code generator will emit calls to the usual __floattidf, etc. (Unless of course these Hexagon-optimized routines actually exist somewhere and just haven't been upstreamed to LLVM yet.)

@androm3da
Copy link
Contributor Author

(Unless of course these Hexagon-optimized routines actually exist somewhere and just haven't been upstreamed to LLVM yet.)

I think this may be the case. I'll check and update the PR soon.

@alexrp
Copy link
Member

alexrp commented Nov 23, 2024

Ok, given llvm/llvm-project#117423, I think this PR is good to go after the remaining align(32) suggestions are accepted.

Co-authored-by: Alex Rønne Petersen <[email protected]>
@androm3da
Copy link
Contributor Author

Ok, given llvm/llvm-project#117423, I think this PR is good to go after the remaining align(32) suggestions are accepted.

Ok - I've taken those, thanks.

re: llvm/llvm-project#117423 -- if it turns out that it's not preferred to remove those, then I'll probably add the specializations to compiler-rt instead (and then corresponding ones here).

@alexrp alexrp enabled auto-merge (squash) November 23, 2024 16:01
@alexrp alexrp merged commit 4aa6246 into ziglang:master Nov 24, 2024
10 checks passed
alexrp added a commit that referenced this pull request Nov 24, 2024
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.

Port Hexagon-specific compiler-rt routines to Zig
3 participants