This is an archive of the discontinued LLVM Phabricator instance.

[llvm] Add initial support for -meabi flag
ClosedPublic

Authored by tinti on Aug 27 2015, 1:29 PM.

Details

Summary

"GCC requires the freestanding environment provide memcpy, memmove, memset
and memcmp" [1].

Hence in GNUEABI targets LLVM should not convert 'memops' to their equivalent
'__aeabi_memops'. This convertion violates GCC contract.

The -meabi flag controls whether or not LLVM will modify 'memops' in GNUEABI
targets.

Without -meabi: use the triple default EABI.
With -meabi=default: use the triple default EABI.
With -meabi=gnu: use 'memops'.
With -meabi=4 or -meabi=5: use '__aeabi_memops'.
With -meabi set to an unknown value: same as -meabi=default.

[1] https://gcc.gnu.org/onlinedocs/gcc-5.2.0/gcc/Standards.html

Diff Detail

Event Timeline

tinti retitled this revision from to [ARM] Separate RTLIB for EABI and GNUEABI.Aug 27 2015, 1:29 PM
tinti updated this object.
tinti added reviewers: rengolin, zatrazz.
tinti added subscribers: behanw, mcharleb, dl9pf.
tinti updated this revision to Diff 33353.Aug 27 2015, 1:29 PM
tinti set the repository for this revision to rL LLVM.Aug 27 2015, 1:29 PM
compnerd edited edge metadata.Aug 27 2015, 7:48 PM

I don't think that this is right. "gnueabi" isn't its own RT, but rather glibc + eabi (vs something like musleabi or androideabi). This is something which really should be fixed in glibc. I have a patch outstanding for adding -mmin-libc-version to allow us to specify the libc version (though, that has differing opinions on representation) that we could use to disable some of these on a version by version basis to accommodate older release of glibc.

I don't think that this is right. "gnueabi" isn't its own RT, but rather glibc + eabi (vs something like musleabi or androideabi). This is something which really should be fixed in glibc.

Ok. Does this mean that the __aeabi_memcpy and family should not be missing on glibc? As far as I saw gcc will never emit them.
Would not be acceptable for LLVM to not emit them while this does not get fixed?

One drawback that @zatrazz pointed out is that this might break some tools that search for __aeabi_memcpy to decide which triple they are running.

Moreover what about adding an hidden flag for disabling __aeabi_memcpy for now? This for sure will cause zero breakage.

I have a patch outstanding for adding -mmin-libc-version to allow us to specify the libc version (though, that has differing opinions on representation) that we could use to disable some of these on a version by version basis to accommodate older release of glibc.

So are you proposing to disable it only in glibc greater than x.y.z?

zatrazz edited edge metadata.Aug 28 2015, 1:26 PM

I don't think that this is right. "gnueabi" isn't its own RT, but rather glibc + eabi (vs something like musleabi or androideabi). This is something which really should be fixed in glibc.

Ok. Does this mean that the __aeabi_memcpy and family should not be missing on glibc? As far as I saw gcc will never emit them.
Would not be acceptable for LLVM to not emit them while this does not get fixed?

GLIBC does provides __aebi_xxx symbols, just check if this build and runs fine:

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <assert.h>

void __aeabi_memcpy (void *dest, const void *src, size_t n);

int main ()
{

char *buffer = (char*) malloc (4096);
memset (buffer       , 'a', 2048);
memset (buffer + 2048, 'b', 2048);
__aeabi_memcpy (buffer, buffer+2048, 2048);
assert (memcmp (buffer, buffer + 2048, 2048) == 0);

return 0;

}

What GLIBC does not provide (and prob won't without a good reason) is to support the __fp16 (16-bit float precision) transformation support, since they are supported by libgcc.

You noted 'RTLIB GNUEABI does not implement all helper functions provided in RTLIB EABI.'. Could you care to explain which symbols are they and which issue exactly are you trying to fix?

One drawback that @zatrazz pointed out is that this might break some tools that search for __aeabi_memcpy to decide which triple they are running.

Moreover what about adding an hidden flag for disabling __aeabi_memcpy for now? This for sure will cause zero breakage.

I have a patch outstanding for adding -mmin-libc-version to allow us to specify the libc version (though, that has differing opinions on representation) that we could use to disable some of these on a version by version basis to accommodate older release of glibc.

So are you proposing to disable it only in glibc greater than x.y.z?

tinti added a comment.Aug 29 2015, 6:52 AM

I don't think that this is right. "gnueabi" isn't its own RT, but rather glibc + eabi (vs something like musleabi or androideabi). This is something which really should be fixed in glibc.

Ok. Does this mean that the __aeabi_memcpy and family should not be missing on glibc? As far as I saw gcc will never emit them.
Would not be acceptable for LLVM to not emit them while this does not get fixed?

GLIBC does provides __aebi_xxx symbols, just check if this build and runs fine:

Yes. When I mean __aeabi_memcpy and family I mean memcpy, memmove and memset.

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <assert.h>

void __aeabi_memcpy (void *dest, const void *src, size_t n);

int main ()
{
  char *buffer = (char*) malloc (4096);
  memset (buffer       , 'a', 2048);
  memset (buffer + 2048, 'b', 2048);
  __aeabi_memcpy (buffer, buffer+2048, 2048);
  assert (memcmp (buffer, buffer + 2048, 2048) == 0);

  return 0;
}

Weird. I have not tested that. I was checking procedures that are lowered to memcpy. Please refer to the one below:

struct my_s {
  unsigned long a[18];
};

void foo(unsigned long *t) {
  *(struct my_s *)t = *((struct my_s *)(1UL));
}

As far as I tested gcc won't lower this to __aeabi_memcpy versions.

What GLIBC does not provide (and prob won't without a good reason) is to support the __fp16 (16-bit float precision) transformation support, since they are supported by libgcc.

You noted 'RTLIB GNUEABI does not implement all helper functions provided in RTLIB EABI.'. Could you care to explain which symbols are they and which issue exactly are you trying to fix?

Yes. We have an issue on LLVMLinux that which is that LLVM converts procedures to __aeabi_memcpy and family. The linker does not recognize them. It gives the following error:

foo.c:(.text+0x4c8): undefined reference to `__aeabi_memcpy'`

Which is fixed by this patch:

If you google a bit you will note that several projects have this issue and a common fix for that is to manually define __aeabi_ops.
By looking at gcc code I have derived this __aeabi_ops:

# from https://github.com/gcc-mirror/gcc/blob/master/gcc/config/arm/arm.c#L2341
__aeabi_d2f
__aeabi_d2iz
__aeabi_d2lz
__aeabi_d2uiz
__aeabi_d2ulz
__aeabi_dadd
__aeabi_dcmpeq
__aeabi_dcmpge
__aeabi_dcmpgt
__aeabi_dcmple
__aeabi_dcmplt
__aeabi_dcmpun
__aeabi_ddiv
__aeabi_dmul
__aeabi_dneg
__aeabi_dsub
__aeabi_f2d
__aeabi_f2iz
__aeabi_f2lz
__aeabi_f2uiz
__aeabi_f2ulz
__aeabi_fadd
__aeabi_fcmpeq
__aeabi_fcmpge
__aeabi_fcmpgt
__aeabi_fcmple
__aeabi_fcmplt
__aeabi_fcmpun
__aeabi_fdiv
__aeabi_fmul
__aeabi_fneg
__aeabi_fsub
__aeabi_i2d
__aeabi_i2f
__aeabi_idiv
__aeabi_idivmod
__aeabi_l2d
__aeabi_l2f
__aeabi_lasr
__aeabi_lcmp
__aeabi_ldivmod
__aeabi_llsl
__aeabi_llsr
__aeabi_lmul
__aeabi_ui2d
__aeabi_ui2f
__aeabi_uidiv
__aeabi_uidivmod
__aeabi_ul2d
__aeabi_ul2f
__aeabi_ulcmp
__aeabi_uldivmod

In LLVM:

# from lib/Target/ARM/ARMISelLowering.cpp
__aeabi_d2f
__aeabi_d2h
__aeabi_d2iz
__aeabi_d2lz
__aeabi_d2uiz
__aeabi_d2ulz
__aeabi_dadd
__aeabi_dcmpeq
__aeabi_dcmpge
__aeabi_dcmpgt
__aeabi_dcmple
__aeabi_dcmplt
__aeabi_dcmpun
__aeabi_ddiv
__aeabi_dmul
__aeabi_dsub
__aeabi_f2d
__aeabi_f2iz
__aeabi_f2lz
__aeabi_f2uiz
__aeabi_f2ulz
__aeabi_fadd
__aeabi_fcmpeq
__aeabi_fcmpge
__aeabi_fcmpgt
__aeabi_fcmple
__aeabi_fcmplt
__aeabi_fcmpun
__aeabi_fdiv
__aeabi_fmul
__aeabi_fsub
__aeabi_i2d
__aeabi_i2f
__aeabi_idiv
__aeabi_l2d
__aeabi_l2f
__aeabi_lasr
__aeabi_ldivmod
__aeabi_llsl
__aeabi_llsr
__aeabi_lmul
__aeabi_memcpy
__aeabi_memmove
__aeabi_memset
__aeabi_ui2d
__aeabi_ui2f
__aeabi_uidiv
__aeabi_ul2d
__aeabi_ul2f
__aeabi_uldivmod

If you check the diff you will se that these functions are only in LLVM:

__aeabi_memcpy
__aeabi_memmove
__aeabi_memset
__aeabi_d2h

One drawback that @zatrazz pointed out is that this might break some tools that search for __aeabi_memcpy to decide which triple they are running.

Moreover what about adding an hidden flag for disabling __aeabi_memcpy for now? This for sure will cause zero breakage.

I have a patch outstanding for adding -mmin-libc-version to allow us to specify the libc version (though, that has differing opinions on representation) that we could use to disable some of these on a version by version basis to accommodate older release of glibc.

So are you proposing to disable it only in glibc greater than x.y.z?

arm-linux-gnueabihf-ld -EL -p --no-undefined -X --pic-veneer --build-id -o .tmp_vmlinux1 -T ./arch/arm/kernel/vmlinux.lds arch/arm/kernel/head.o init/built-in.o --start-group usr/built-in.o arch/arm/vfp/built-in.o arch/arm/kernel/built-in.o arch/arm/mm/built-in.o arch/arm/common/built-in.o arch/arm/probes/built-in.o arch/arm/net/built-in.o arch/arm/crypto/built-in.o arch/arm/firmware/built-in.o arch/arm/mach-vexpress/built-in.o arch/arm/plat-versatile/built-in.o kernel/built-in.o mm/built-in.o fs/built-in.o ipc/built-in.o security/built-in.o crypto/built-in.o block/built-in.o arch/arm/lib/lib.a lib/lib.a arch/arm/lib/built-in.o lib/built-in.o drivers/built-in.o sound/built-in.o firmware/built-in.o arch/arm/oprofile/built-in.o net/built-in.o --end-group
arch/arm/kernel/built-in.o: In function `copy_thread':
./src/linux/arch/arm/kernel/process.c:214: undefined reference to `__aeabi_memcpy4'

The __aeabi_memcpy can just use memcpy. You can alias __aeabi_memcpy4 and __aeabi_memcpy8 to memcpy. The latter two are merely optimizations, and falling back to the unaligned use of memcpy is technically correct at a slight performance cost (which you would end up paying with your original suggestion anyways). Similarly, __aeabi_memset can use memset, and __aeabi_memmove can use memmove. That leaves one function: __aeabi_d2h which is just a cast for a double to a uint16_t. For your particular use, I would recommend providing implementations within the kernel, if you are targeting EABI, all of which would be quite trivial, and would really have negligible costs for maintenance.

tinti added a comment.Aug 30 2015, 9:29 AM

The __aeabi_memcpy can just use memcpy. You can alias __aeabi_memcpy4 and __aeabi_memcpy8 to memcpy. The latter two are merely optimizations, and falling back to the unaligned use of memcpy is technically correct at a slight performance cost (which you would end up paying with your original suggestion anyways). Similarly, __aeabi_memset can use memset, and __aeabi_memmove can use memmove.

I agree. Still I believe this don't fix my problem only. It is a wider solution. If we don't implement such behavior in LLVM every project that uses it must implement these functions if it ever produce something that will link to gcc. That is sad and too unclear.

On the other hand if we apply this patch it will be a single behavior/implementation across all projects. Which sounds reasonable for me (or as I said before to have a hidden flag that controls this behavior).

That leaves one function: __aeabi_d2h which is just a cast for a double to a uint16_t. For your particular use, I would recommend providing implementations within the kernel, if you are targeting EABI, all of which would be quite trivial, and would really have negligible costs for maintenance.

Being not so nice this one is not a big issue in my case and I don't believe that many people have problem with this one (at least by googling).

rengolin edited edge metadata.Aug 30 2015, 9:37 AM

I agree. Still I believe this don't fix my problem only. It is a wider solution. If we don't implement such behavior in LLVM every project that uses it must implement these functions if it ever produce something that will link to gcc. That is sad and too unclear.

There is already such hack in Android, and I'd not like to infer that kind of behaviour in any project.

If this is one of those "to each its own", than I think it's the kind of discussion that we should be having in the GCC list, too.

That leaves one function: __aeabi_d2h which is just a cast for a double to a uint16_t. For your particular use, I would recommend providing implementations within the kernel, if you are targeting EABI, all of which would be quite trivial, and would really have negligible costs for maintenance.

Being not so nice this one is not a big issue in my case and I don't believe that many people have problem with this one (at least by googling).

Good, I also agree with Saleem in this one.

This is a problem that arose from Vinicius investigating all calls to make sure we weren't forgetting anything, and not finding d2h in GCC. Whatever decision we take on this one should be ok, at least for the time being.

However, as @zatrass pointed out, glibc already has these functions (look under #/sysdeps/arm/aeabi_mem*.c). Well, I can find a number of cases where people are hitting issues with the normal libcalls as well (to libgcc/compiler-rt), so is your contention that we shouldn't use those either? The fact of the matter is that you are working in a freestanding environment, and that means you need to provide the supporting library calls. If you don't wish to do so, you can always link against libclang_rt.builtins-arm.a or libgcc.a. This really is only a problem for a freestanding environment, not a general issue, which is why I am suggesting that the user bear the burden on this case.

However, as @zatrass pointed out, glibc already has these functions (look under #/sysdeps/arm/aeabi_mem*.c).

Neither the Kernel, nor Android use glibc.

Well, I can find a number of cases where people are hitting issues with the normal libcalls as well (to libgcc/compiler-rt), so is your contention that we shouldn't use those either?

No, my point is just that we shouldn't assume people have glibc or bionic or whatever, but we can assume that people have compiler-rt or libgcc.

The fact of the matter is that you are working in a freestanding environment, and that means you need to provide the supporting library calls. If you don't wish to do so, you can always link against libclang_rt.builtins-arm.a or libgcc.a.

No, you can't and that's the problem. Free standing shouldn't *have* to link against anything, especially when the *compiler* is changing the function name without the user's explicit request.

This really is only a problem for a freestanding environment, not a general issue, which is why I am suggesting that the user bear the burden on this case.

If the compiler wasn't doing anything wrong, I would surely agree with you. But we're changing the names from "memcpy" to "aeabi_memcpy", then to "aeabi_memcpy4" and requesting the user to have all of them pointing back to "memcpy", which is what they requested in the first place.

The two alternatives I could think of were:

  1. Make sure that there is at least one flag that won't change the function calls into other things. The user will lose optimization opportunities, but such is life.
  1. Make sure to be compatible to at least one ABI. If GNU tools don't change the names of A, B and C functions in a free standing environment, so shouldn't we. This will lead to many special cases inside the compiler, but such is life.

I honestly don't know which one is best. From what I can see, the first one is easier and quicker to fix, but may lead to odd behaviour in the future (ie. goo-ization of code), while the second will lead to immediate crap code going in, but not growing later.

But no matter how crap either solutions are, forcing the users to write a special file that "implements" __aeabi_memcpy as a call to memcpy is worse.

Of course, I'd love to hear of better solutions. :)

--renato

However, as @zatrass pointed out, glibc already has these functions (look under #/sysdeps/arm/aeabi_mem*.c).

Neither the Kernel, nor Android use glibc.

True, but we already have that information in the case of android (androideabi indicates that they are using bionic).

Well, I can find a number of cases where people are hitting issues with the normal libcalls as well (to libgcc/compiler-rt), so is your contention that we shouldn't use those either?

No, my point is just that we shouldn't assume people have glibc or bionic or whatever, but we can assume that people have compiler-rt or libgcc.

I see. Well, clang_rt.builtins-arm.a does provide these already. Perhaps we should ask the GCC folks to consider moving the aliases from glibc into libgcc?

The fact of the matter is that you are working in a freestanding environment, and that means you need to provide the supporting library calls. If you don't wish to do so, you can always link against libclang_rt.builtins-arm.a or libgcc.a.

No, you can't and that's the problem. Free standing shouldn't *have* to link against anything, especially when the *compiler* is changing the function name without the user's explicit request.

That seems to be where the disconnect is, it is with the user's explicit request. Using the "gnueabi" environment, *is* the request.

This really is only a problem for a freestanding environment, not a general issue, which is why I am suggesting that the user bear the burden on this case.

If the compiler wasn't doing anything wrong, I would surely agree with you. But we're changing the names from "memcpy" to "aeabi_memcpy", then to "aeabi_memcpy4" and requesting the user to have all of them pointing back to "memcpy", which is what they requested in the first place.

I think you misunderstood my statement. It is not changing __aeabi_memcpy to __aeabi_memcpy4. The latter is an optimization: you know that the pointer is 4-byte aligned. Do you need to have the optimized version? No, but you pay a slight penalty. If you don't want to enable the optimization, just alias __aeabi_memcpy4 to memcpy, otherwise, provide the optimized implementation.

The two alternatives I could think of were:

  1. Make sure that there is at least one flag that won't change the function calls into other things. The user will lose optimization opportunities, but such is life.

There already is: use the armv7-linux-gnu environment, because you don't wish to conform to the EABI RTABI. You won't get the libcall functions "changed into other things". If this has an issue, we should absolutely fix that.

  1. Make sure to be compatible to at least one ABI. If GNU tools don't change the names of A, B and C functions in a free standing environment, so shouldn't we. This will lead to many special cases inside the compiler, but such is life.

It is compatible with one ABI: EABI RTABI. Now, unfortunately, we do often emulate gcc's behaviors, and if there is something special about -ffreestanding that gcc does, then I agree we should do so. However, if its just that gcc doesn't implement the EABI support as well, I don't see why we should be forced to do the same.

I honestly don't know which one is best. From what I can see, the first one is easier and quicker to fix, but may lead to odd behaviour in the future (ie. goo-ization of code), while the second will lead to immediate crap code going in, but not growing later.

But no matter how crap either solutions are, forcing the users to write a special file that "implements" __aeabi_memcpy as a call to memcpy is worse.

Of course, I'd love to hear of better solutions. :)

--renato

I see. Well, clang_rt.builtins-arm.a does provide these already. Perhaps we should ask the GCC folks to consider moving the aliases from glibc into libgcc?

We're contemplating this one. Vinicius will prepare an email.

That seems to be where the disconnect is, it is with the user's explicit request. Using the "gnueabi" environment, *is* the request.

If "GNUEABI" meant something, that would be a request. But it doesn't.

Or rather, it means "whatever GCC does with the EABI", which is a pretty poor description, and even so, we're not doing "it".

There already is: use the armv7-linux-gnu environment, because you don't wish to conform to the EABI RTABI. You won't get the libcall functions "changed into other things". If this has an issue, we should absolutely fix that.

Right, this is yet another solution, and one that can be tested right now! Vinicius, does changing the triple helps the kernel?

It may not help the "androideabi", but not only Android has bionic in which they can add any hack they want, but also the control the ABI themselves, so they can change where those functions are declared / instantiated.

It is compatible with one ABI: EABI RTABI. Now, unfortunately, we do often emulate gcc's behaviors, and if there is something special about -ffreestanding that gcc does, then I agree we should do so. However, if its just that gcc doesn't implement the EABI support as well, I don't see why we should be forced to do the same.

Because that's what it means to follow the GNU version of the EABI support, "gnueabi". Emulation has to be done bug by bug. :)

I see. Well, clang_rt.builtins-arm.a does provide these already. Perhaps we should ask the GCC folks to consider moving the aliases from glibc into libgcc?

We're contemplating this one. Vinicius will prepare an email.

That seems to be where the disconnect is, it is with the user's explicit request. Using the "gnueabi" environment, *is* the request.

If "GNUEABI" meant something, that would be a request. But it doesn't.

Or rather, it means "whatever GCC does with the EABI", which is a pretty poor description, and even so, we're not doing "it".

There already is: use the armv7-linux-gnu environment, because you don't wish to conform to the EABI RTABI. You won't get the libcall functions "changed into other things". If this has an issue, we should absolutely fix that.

Right, this is yet another solution, and one that can be tested right now! Vinicius, does changing the triple helps the kernel?

Yes, it does. By using armv7-linux-gnu the memcpy is not lowered to __aeabi_memcpy.

It may not help the "androideabi", but not only Android has bionic in which they can add any hack they want, but also the control the ABI themselves, so they can change where those functions are declared / instantiated.

It is compatible with one ABI: EABI RTABI. Now, unfortunately, we do often emulate gcc's behaviors, and if there is something special about -ffreestanding that gcc does, then I agree we should do so. However, if its just that gcc doesn't implement the EABI support as well, I don't see why we should be forced to do the same.

Because that's what it means to follow the GNU version of the EABI support, "gnueabi". Emulation has to be done bug by bug. :)

I see. Well, clang_rt.builtins-arm.a does provide these already. Perhaps we should ask the GCC folks to consider moving the aliases from glibc into libgcc?

We're contemplating this one. Vinicius will prepare an email.

That seems to be where the disconnect is, it is with the user's explicit request. Using the "gnueabi" environment, *is* the request.

If "GNUEABI" meant something, that would be a request. But it doesn't.

Or rather, it means "whatever GCC does with the EABI", which is a pretty poor description, and even so, we're not doing "it".

There already is: use the armv7-linux-gnu environment, because you don't wish to conform to the EABI RTABI. You won't get the libcall functions "changed into other things". If this has an issue, we should absolutely fix that.

Right, this is yet another solution, and one that can be tested right now! Vinicius, does changing the triple helps the kernel?

It may not help the "androideabi", but not only Android has bionic in which they can add any hack they want, but also the control the ABI themselves, so they can change where those functions are declared / instantiated.

It is compatible with one ABI: EABI RTABI. Now, unfortunately, we do often emulate gcc's behaviors, and if there is something special about -ffreestanding that gcc does, then I agree we should do so. However, if its just that gcc doesn't implement the EABI support as well, I don't see why we should be forced to do the same.

Because that's what it means to follow the GNU version of the EABI support, "gnueabi". Emulation has to be done bug by bug. :)

I believe on that too. Otherwise it should not be called by this name.

Because that's what it means to follow the GNU version of the EABI support, "gnueabi". Emulation has to be done bug by bug. :)

I believe on that too. Otherwise it should not be called by this name.

I agree. In the ideal world, the Linux kernel would use the armv7-linux triple rather than armv7-linux-gnu or armv7-linux-gnueabi. The subtle difference between the tree being that the first one makes no assumptions about the underlying C library, while the second one assumes glibc is present (and the kernel here has added supporting behavior for that), and the last being glibc+EABI RTABI support. Unfortunately, I can't say off hand how well we support the first triple. So, I would argue that we already don't call it by that name. I agree that triples are confusing, and we haven't done much to help make this particularly clear to users of clang, but, we didn't create that complexity. If armv7-linux is problematic, we should definitely be addressing those issues. While a slight bit more complex, it allows us to have a better QoI, while being able to support the use case for environments like the Linux kernel.

asl added a subscriber: asl.Sep 4 2015, 3:37 AM

I agree. In the ideal world, the Linux kernel would use the armv7-linux triple rather than armv7-linux-gnu or armv7-linux-gnueabi. The subtle difference between the tree being that the first one makes no assumptions about the underlying C library, while the second one assumes glibc is present (and the kernel here has added supporting behavior for that), and the last being glibc+EABI RTABI support. Unfortunately, I can't say off hand how well we support the first triple. So, I would argue that we already don't call it by that name. I agree that triples are confusing, and we haven't done much to help make this particularly clear to users of clang, but, we didn't create that complexity. If armv7-linux is problematic, we should definitely be addressing those issues. While a slight bit more complex, it allows us to have a better QoI, while being able to support the use case for environments like the Linux kernel.

Unfortunately, changing triples for GNU toolchains, or many build systems around kernel, Android, CI automation, etc is not as easy as it is for us.

As I said on the email thread, GNUEABI doesn't make GCC generate EABI calls, just accept them. If we are to implement the GNUEABI as is *is*, we shouldn't generate them either.

Not to mention breaking the ISO C contract that GCC has with freestanding environments, which is to allow them to implement mem* calls and not fuss about it.

tinti updated this revision to Diff 34168.Sep 7 2015, 11:59 AM
tinti retitled this revision from [ARM] Separate RTLIB for EABI and GNUEABI to [ARM] Make RTLIB for GNUEABI compliant with GCC standards.
tinti updated this object.
tinti edited edge metadata.
tinti removed rL LLVM as the repository for this revision.

Update patch and reword properly.

The main argument for this patch is now that LLVM is violating GCC contract in GNUEABI targets.

Because that's what it means to follow the GNU version of the EABI support, "gnueabi". Emulation has to be done bug by bug. :)

I believe on that too. Otherwise it should not be called by this name.

I agree. In the ideal world, the Linux kernel would use the armv7-linux triple rather than armv7-linux-gnu or armv7-linux-gnueabi. The subtle difference between the tree being that the first one makes no assumptions about the underlying C library, while the second one assumes glibc is present (and the kernel here has added supporting behavior for that), and the last being glibc+EABI RTABI support. Unfortunately, I can't say off hand how well we support the first triple. So, I would argue that we already don't call it by that name. I agree that triples are confusing, and we haven't done much to help make this particularly clear to users of clang, but, we didn't create that complexity. If armv7-linux is problematic, we should definitely be addressing those issues. While a slight bit more complex, it allows us to have a better QoI, while being able to support the use case for environments like the Linux kernel.

The problem I see with current clang approach regarding the memcpy/memset/memmove call generation for auto-generated code
(struct copy, loop optimization, etc) are:

  1. GCC currently uses ISO C memcpy/memset/memmove calls consistently over all architectures, even for ARM with eabi or gnueabi. This leads to freestanding to have special handling for ARM + (gnu)eabi and add unnecessary complexity.
  2. AEABI mem* operation have different prototype than the ISO C counterparts and for a project to support them it will require to either implement the symbols itself or create wrappers (which are the simpler solution and usually the solution taken for most projects);
  3. AEABI mem* is only an optimization if either the runtime or freestanding projects implements the correct symbol, which is not the case for most projects (kernel, Linux C-runtimes). In these case it will be just an unnecessary function call.
  4. It might a problem to mix objects compiled with different toolchain with different requirements. For instance, if you build a static library with clang and link against another one built with GCC in a freestanding environment or against a C-runtime that does not provide the AEABI mem* operations (musl, for instance, only implemented them two weeks ago).

So I do not see any compelling reason to emit AEABI mem* call on gnueabi (I can argue that even for eabi, but they might link against specific runtime that do support optimized AEABI mem implementations).

So, after a discussion with @rengolin, I think I would prefer that we handle this slightly differently; implementing this is reasonable as the "GNU" variant of the EABI, which is controlled via the -meabi option, which takes one of three values (gnu, 4, or 5). Under the gnu variant, we should produce the same code as GCC. Defaulting -meabi to gnu would be reasonable, which would mean that although you can still control this, the default behaviour would be identical to GCC, as that is what the rest of clang does.

So, after a discussion with @rengolin, I think I would prefer that we handle this slightly differently; implementing this is reasonable as the "GNU" variant of the EABI, which is controlled via the -meabi option, which takes one of three values (gnu, 4, or 5). Under the gnu variant, we should produce the same code as GCC. Defaulting -meabi to gnu would be reasonable, which would mean that although you can still control this, the default behaviour would be identical to GCC, as that is what the rest of clang does.

I can live with that. :)

Also, seems that GCC is more adhering to the EABI than I thought, and only keeps the ISO C names for memcpy/memset/memcmp calls. I think that's a reasonable compromise, too.

In this case, the "-meabi=gnu" is EABI calls for all builtins except memory ones.

cheers,
--renato

So, after a discussion with @rengolin, I think I would prefer that we handle this slightly differently; implementing this is reasonable as the "GNU" variant of the EABI, which is controlled via the -meabi option, which takes one of three values (gnu, 4, or 5). Under the gnu variant, we should produce the same code as GCC. Defaulting -meabi to gnu would be reasonable, which would mean that although you can still control this, the default behaviour would be identical to GCC, as that is what the rest of clang does.

I can live with that. :)

Yay!

In this case, the "-meabi=gnu" is EABI calls for all builtins except memory ones.

Sounds good to me.

tinti updated this revision to Diff 34262.Sep 8 2015, 2:16 PM
tinti retitled this revision from [ARM] Make RTLIB for GNUEABI compliant with GCC standards to [ARM] Add -arm-eabi for controlling RTLIB.
tinti updated this object.

Update according to @compnerd and @rengolin suggestions.

compnerd added inline comments.Sep 8 2015, 3:22 PM
lib/Target/ARM/ARMISelLowering.cpp
73

This is the wrong spelling. This option is not arm specific, but applies to PPC, MIPS, and ARM. This should be done in the frontend (clang), and passed along, and we can add the option to the other tools as necessary (similar to -march, -mcpu, ...).

tinti added inline comments.Sep 8 2015, 3:27 PM
lib/Target/ARM/ARMISelLowering.cpp
73

D'oh. Sorry I will move it to Clang.

So, I agree, the flag should be in Clang, but we can't use -meabi there, as GCC only uses that flag internally (CC1) and GAS also accepts it, so we don't want to create a flag that has the same name but different behaviour.

The best way of doing this is to create a target-feature in Clang and pass it down to LLVM like the others. But we have to do in reverse order.

First, you implement the target-feature in LLVM, and make this change. Then, you add the feature in Clang and make sure we're passing it down and that it's changing the behaviour by having some memcpy tests in Clang. The last step is to come up with a way to change the flag from the Clang driver, not CC1.

I think that llc (maybe llvm-mc) should have that flag, too, just like GAS, and that it should be called -meabi and have the same semantics. I'm ok with this being an llc-only flag in the first step of the changes, as that allows us to test it. Otherwise, this wouldn't work for C->IR->ASM route as it would for the C->ASM route.

lib/Target/ARM/ARMISelLowering.cpp
379

Couldn't you move this condition before the struct declaration, like the one above?

test/CodeGen/ARM/fp16-promote.ll
272 ↗(On Diff #34262)

Wait, why are these tests still changed? We're only changing memcpy & al.

tinti added inline comments.Sep 10 2015, 4:23 PM
lib/Target/ARM/ARMISelLowering.cpp
73

Agreed.

379

Fair enough.

test/CodeGen/ARM/fp16-promote.ll
272 ↗(On Diff #34262)

My bad. They should not be here.

tinti updated this revision to Diff 38571.Oct 27 2015, 11:09 AM
tinti updated this object.

Create -eabi flag to control EABI version.

tinti added inline comments.Oct 27 2015, 11:12 AM
test/CodeGen/ARM/fp16-promote.ll
272 ↗(On Diff #34262)
tinti retitled this revision from [ARM] Add -arm-eabi for controlling RTLIB to [llvm] Add initial support for -eabi flag.Oct 29 2015, 12:46 PM
rengolin added a subscriber: jroelofs.

Hi Vinicius,

This looks good to me. @jroelofs @compnerd @t.p.northover, can you see anything that might impact your workloads?

cheers,
--renato

test/CodeGen/ARM/arm-eabi.ll
2

You don't need the prefix=CHECK, it's implied by default.

tinti updated this revision to Diff 38801.Oct 30 2015, 6:31 AM

Add context

rengolin added inline comments.Nov 3 2015, 4:54 AM
include/llvm/Target/TargetOptions.h
64

This comment is out of place

jroelofs added inline comments.Nov 3 2015, 7:51 AM
test/CodeGen/ARM/memfunc.ll
14

note that this check matches both __aeabi_memmove and memmove. Is that desirable, or should these be {{ memmove}}?

compnerd added inline comments.Nov 3 2015, 7:54 AM
include/llvm/Target/TargetOptions.h
61

Just convert this to an enum class instead of the namespace and enum.

64

EABI is an initialism, please make it uppercase.

rengolin added inline comments.Nov 3 2015, 8:45 AM
include/llvm/Target/TargetOptions.h
61

This doesn't seem to be the pattern anywhere else... I agree it's repetitive, but it seems intentional?

test/CodeGen/ARM/memfunc.ll
14

good point!

tinti added inline comments.Nov 3 2015, 1:33 PM
include/llvm/Target/TargetOptions.h
61

I prefer to follow the patterns unless we agree to change it in all.

On Clang this pattern seems to be the way you suggested @compnerd.

64

@rengolin Agreed
@compnerd Agreed

64

Agreed.

test/CodeGen/ARM/memfunc.ll
14

@jroelofs, @rengolin indeed a good point. ARM's RTABI document says [1]:

... None of the three functions is required to return anything in r0.

Each of these functions can be smaller or faster than the general memcpy or each can be an alias for memcpy itself, similarly for memmove.

Compilers can replace calls to memcpy with calls to one of these functions if they can deduce that the constraints are satisfied. For example, any memcpy whose return value is ignored can be replaced with \_\_aeabi_memcpy. If the copy is between 4-byte-aligned pointers it can be replaced with \_\_aeabi_memcpy4, and so on.

These functions have void as a return. So the compiler is free to decide if it will or not replace them. I believe that the missing point in these tests is the non-void return versions.

I would like to add them. What do you think?

[1] http://infocenter.arm.com/help/topic/com.arm.doc.ihi0043d/IHI0043D_rtabi.pdf.

compnerd added inline comments.Nov 3 2015, 6:49 PM
include/llvm/Target/TargetOptions.h
61

Changing the other instances here is just going to cause noise. However, for newer instances, it makes sense to use the newer functionality. It also means that the compiler enforces type safety, so it is more than just style.

tinti added inline comments.Nov 4 2015, 2:19 PM
include/llvm/Target/TargetOptions.h
61

@compnerd Agreed. As you pointed out a gain I think it is reasonable to do so.

tinti updated this revision to Diff 39301.Nov 4 2015, 6:21 PM
tinti set the repository for this revision to rL LLVM.
  • Use enum class.
  • Clang format (except in two places for readability).
tinti marked 11 inline comments as done.Nov 4 2015, 6:23 PM
tinti marked an inline comment as done.Nov 4 2015, 6:23 PM

As @jroelofs mentioned, the function matches should be stricter since they can match the alternate versions. LG otherwise.

include/llvm/CodeGen/CommandFlags.h
12

Might be nice to spell this as meabi to keep it consistent across clang and opt.

rengolin added inline comments.Nov 5 2015, 8:30 AM
include/llvm/CodeGen/CommandFlags.h
12

Yes, I was going to mention this and forgot. I agree.

tinti updated this revision to Diff 39393.Nov 5 2015, 12:23 PM
  • Change flag to "meabi" to be keep consistence with Clang.
  • Use full call for memops functions to not be missinterpreted from memops functions.
  • Remove extra CHECK.
tinti marked 4 inline comments as done.Nov 5 2015, 12:23 PM
tinti marked 2 inline comments as done.Nov 5 2015, 12:26 PM
rengolin accepted this revision.Nov 5 2015, 12:37 PM
rengolin edited edge metadata.

Hi Vinicius,

This now looks good to me. Thanks!

You may have to change your clang patch, too, due to the change in -meabi here.

This revision is now accepted and ready to land.Nov 5 2015, 12:37 PM
tinti retitled this revision from [llvm] Add initial support for -eabi flag to [llvm] Add initial support for -meabi flag.Nov 5 2015, 12:38 PM
tinti updated this object.
tinti edited edge metadata.
tinti added a comment.Nov 5 2015, 12:42 PM

Hi Vinicius,

This now looks good to me. Thanks!

You may have to change your clang patch, too, due to the change in -meabi here.

In fact no because it sets the CodeGenOpts directly. But you remember me to reword the patch text!

Thanks.

tinti updated this revision to Diff 39595.Nov 6 2015, 2:45 PM
tinti updated this object.
tinti updated this object.
rengolin closed this revision.Nov 9 2015, 4:43 AM

r252462