compiler-rt: add aeabi_fcmp, comparesf2 #2299
Conversation
| std.math.inf(f32), | ||
| }; | ||
|
|
||
| fn generateVector(comptime a: f32, comptime b: f32) TestVector { |
There was a problem hiding this comment.
I had to make these function parameters comptime in order to use the function in the compile time initialization of the test_vectors array at line 91. The error said that the parameters needed to be comptime known, which they were because I was passing them in at compile time so I'm confused. I tried adding inline in front of the for loops and that didn't make a difference. Is this a bug?
In the documentation a function without comptime parameters is used at compile time but it is not contained within a block, which I suspect is the difference.
There was a problem hiding this comment.
For functions that return generics, the return type should be type.
There was a problem hiding this comment.
I don't think this function is supposed to return a generic? It's supposed to return a struct with the values filled out, not instantiate a struct type.
Am i understanding what you mean?
There was a problem hiding this comment.
Here is the relevant documentation:
https://ziglang.org/documentation/master/#Compile-Time-Expressions
See the section:
const first_25_primes = firstNPrimes(25);
const sum_of_first_25_primes = sum(first_25_primes);There was a problem hiding this comment.
Thanks for the fast replies -- misread generateVector as genericVector!
There was a problem hiding this comment.
Is this a bug?
That does sound like a bug. Before I merge this let me play around with it a bit and see if it's something I can fix along with the merge.
| @@ -636,10 +636,12 @@ set(ZIG_STD_FILES | |||
| "special/build_runner.zig" | |||
| "special/builtin.zig" | |||
| "special/compiler_rt.zig" | |||
| "special/compiler_rt/arm/aeabi_fcmp.zig" | |||
There was a problem hiding this comment.
I don't think we need a new directory for arm.
aeabi_fcmp doesn't seem to conflict with any other namespace.
There was a problem hiding this comment.
I'm following the directory structure of compiler-rt.
How close should the port of compiler-rt be?
There was a problem hiding this comment.
Opinion: up until now we don't have sub-directories, so I think it should be flat, unless re run into namespace conflicts.
There was a problem hiding this comment.
I tend to like flat directory structures too. It's already pretty nested, but nesting it makes the intention to follow compiler-rt more explicit.
I would like to consider that we don't follow compiler-rt exactly. The hard floating point targets can just have the hard floating point instructions directly in the functions that provide them instead of branching all over the place to functions from other files. Compiler-rt has a complicated (seemingly to me) system for determining which version of a function gets linked. I think compiler-rt is a great starting point, but if we are already doing a port, I would like to be able to reorder/insert inline some stuff. That would make the port for hard float ABI progress at the same time as the soft float ABI. Of course that does make it harder to compare against future releases of compiler-rt.
| // | ||
| // https://github.com/llvm/llvm-project/commit/d674d96bc56c0f377879d01c9d8dfdaaa7859cdb/compiler-rt/lib/builtins/arm/aeabi_fcmp.S | ||
|
|
||
| const compiler_rt_armhf_target = false; // TODO |
There was a problem hiding this comment.
This should be tied to builtin state and become a compile error if not possible.
There was a problem hiding this comment.
That would disable every hard floating point arm32 target because there is no support for hard floating point at all currently and release-fast/release-safe mode include even unused built-ins #2062. Such a change is beyond my area of responsibility. On a different compiler-rt pull request I did I just didn't add the assembly section at all. This time I marked it with a TODO, I'm not sure why I marked it this time.
Because the port of compiler-rt is so large, I think it might make more sense to incrementally add hard floating point support (my current plan), rather than just say it won't work at all. I am working my way through the soft floating point routines I need to work around #2062 first.
I know we have #1290, but maybe the hard floating point support on arm32 targets could be it's own issue so it doesn't get lost in the sauce. It also would document the plan to add it explicitly.
Can i request the opinion of @andrewrk?
There was a problem hiding this comment.
Even without TODOs in the code, its very easy to see what needs to be added from compiler-rt to support hard floating point. Just grep for COMPILER_RT_ARMHF_TARGET.
There was a problem hiding this comment.
The reason I think that should be a compiler error is that if the floatABI is set to hard and we don't deliver hard, then it seems like a fallacy. The @compileError should mention to set the floatABI to soft.
There was a problem hiding this comment.
Would you re-enable the hard float abi once every function was complete? That is a lot of code to wait on.
It would also make testing the addition of each function more difficult as you would have to go through and comment out the compile errors in order to get a binary with the hard float routine you are working on in it.
There was a problem hiding this comment.
Whether or not hard float is desired is available via this logic:
const compiler_rt_armhf_target = switch (builtin.abi) { .gnueabihf, .eabihf, .musleabihf => true, else => false };I get that compiler-rt is incomplete, especially for these targets, but aren't compile errors better than silently incorrect ABI?
|
I'm not sure. I think I would prefer to have a slowly improving build that
at least still works. Compiler-rt (llvm's) is hardly perfect as it stands
now.
I'm pretty sure it's incorrect for arm4t, for example, as lld complains
that arm4t doesn't support the blx instruction.
Compiler-rt is assembled with the arm assembler, I think and I can't
remember the name of it, while Zig is assembled using clangs built-in
assembler. The syntax is different slightly. For example, there is no
`.syntax unified` directive in clang. All assembly is assumed to be using
unified syntax.
The build process for that might use a different linker that can handle the
older architecture. I haven't looked into it. If we're doing a port to Zig,
it needs to work with the build process for Zig as you don't want to
include a separately compiled compiler-rt.
FWIW I don't think it's worth any time to make Zig work on old
architectures pre-armv6. Nobody uses them, certainly not anyone that will
be trying out a newfangled language like Zig.
…On Thu, Apr 18, 2019, 8:49 AM Andrew Kelley ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In std/special/compiler_rt/arm/aeabi_fcmp.zig
<#2299 (comment)>:
> @@ -0,0 +1,108 @@
+// Ported from:
+//
+// llvm/llvm-project@d674d96/compiler-rt/lib/builtins/arm/aeabi_fcmp.S
+
+const compiler_rt_armhf_target = false; // TODO
Whether or not hard float is desired is available via this logic:
const compiler_rt_armhf_target = switch (builtin.abi) { .gnueabihf, .eabihf, .musleabihf => true, else => false };
I get that compiler-rt is incomplete, especially for these targets, but
aren't compile errors better than silently incorrect ABI?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2299 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AJOF5C7VQPEUKWDRI7U2FITPRB4ABANCNFSM4HGWLKJA>
.
|
|
Haha, forget my last comment about v4t. My program had an illegal instruction for that architecture in it, hence the error. I had an inline asm block for testing that I forgot to remove. Also explains why I was unable to find any instruction Everything else in my comment still stands. |
No description provided.