This is an archive of the discontinued LLVM Phabricator instance.

Do not generate calls to the 128-bit function __multi3() on 32-bit ARM.
ClosedPublic

Authored by koutheir on Jun 8 2021, 9:18 AM.

Details

Summary

The function __multi3() is undefined on 32-bit ARM, so a call to it should never be emitted. Instead, plain instructions need to be generated to perform 128-bit multiplications.

Diff Detail

Event Timeline

koutheir created this revision.Jun 8 2021, 9:18 AM
koutheir requested review of this revision.Jun 8 2021, 9:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2021, 9:18 AM
koutheir retitled this revision from Do not generate calls to the 128-bit function __multi3() on 32-bit ARM. The function __multi3() is undefined on 32-bit ARM, so a call to it should never be emitted. Instead, plain instructions need to be generated to perform 128-bit multiplications. to Do not generate calls to the 128-bit function __multi3() on 32-bit ARM..Jun 8 2021, 9:19 AM
koutheir edited the summary of this revision. (Show Details)
koutheir edited the summary of this revision. (Show Details)

This patch is intended to solve the bug 20871 for the ARM back-end.

Doesn't this depend on which runtime library you're using? No Arm32 library implement that?

I remember a patch to add 128-bit arithmetic into Armv7 many years ago (from Android) but I'm not sure it ever got into compiler-rt.

Doesn't this depend on which runtime library you're using? No Arm32 library implement that?

I remember a patch to add 128-bit arithmetic into Armv7 many years ago (from Android) but I'm not sure it ever got into compiler-rt.

I got linking errors due to undefined symbol __multi3.

Looking at compiler-rt/lib/builtins/multi3.c, I see that __multi3() is implemented only if CRT_HAS_128BIT is defined:

#ifdef CRT_HAS_128BIT
/*...*/
// Returns: a * b

COMPILER_RT_ABI ti_int __multi3(ti_int a, ti_int b) {
/*...*/

The file compiler-rt/lib/builtins/int_types.h does not define CRT_HAS_128BIT on ARM:

#if defined(__LP64__) || defined(__wasm__) || defined(__mips64) ||             \
    defined(__riscv) || defined(_WIN64)
#define CRT_HAS_128BIT
#endif

So, I think the answer to your question is no, __multi3() is not implemented for ARM.

Right, ok, this is true for compiler-rt, but not necessarily other runtime libraries.

There is an open discussion if we should assume compiler-rt is the default and use what we have, but this is a different issue.

If the call to __multi3 is there by accident (leaked from other target's default), then it's ok to remove it. If not, we may need some more complex solution.

@compnerd @joerg @t.p.northover @srhines, do you know of any Arm32 runtime library that implements 128-bit maths with Clang?

I think Android does, so perhaps we can add a condition there on AndroidEABI?

Right, ok, this is true for compiler-rt, but not necessarily other runtime libraries.

There is an open discussion if we should assume compiler-rt is the default and use what we have, but this is a different issue.

If the call to __multi3 is there by accident (leaked from other target's default), then it's ok to remove it. If not, we may need some more complex solution.

@compnerd @joerg @t.p.northover @srhines, do you know of any Arm32 runtime library that implements 128-bit maths with Clang?

I think Android does, so perhaps we can add a condition there on AndroidEABI?

We don't define 128-bit operations. https://cs.android.com/search?q=CRT_HAS_128BIT%20-filepath:external%2Fcompiler-rt%20-filepath:external%2Fllvm-project&sq=&ss=android shows that we don't define CRT_HAS_128BIT anywhere, nor do I see any 128-bit symbols in our libclang_rt.builtins-arm-android.a. We do default to compiler-rt these days for builtins, and thus the 128-bit symbols can't be coming from anywhere else either.

joerg added a comment.Jun 8 2021, 12:56 PM

We inherited the lack of int128_t support on 32bit platforms from GCC. IMO we should fix that, the only tricky one is division.

We could change compiler-rt to expose the relevant routines if we wanted to. See https://reviews.llvm.org/D43106 etc.

rengolin accepted this revision.Jun 9 2021, 2:25 AM

Right, I think it's a bit clearer now.

  1. We inherited the lack of 128-bit arithmetic from GCC and other targets, so currently, there isn't a default place we can get them from.
  2. The current version of compiler-rt doesn't have them, nor does libgcc, so if users want them, they have to link their own libs.
  3. We want to implement them, long term, in compiler-rt, which we also want to make the default in LLVM for all supported targets.

Given the time-frame of item (3) and the fact that, today, the compiler is generating code that doesn't link (without users providing the symbols themselves), I think this change is correct for the time being. I'm creating a bug to address this and adding people here to the CC list.

Otherwise, this patch LGTM, thanks!

This revision is now accepted and ready to land.Jun 9 2021, 2:25 AM

@rengolin: I don't have commit access. Can you please commit this patch on my behalf?

This revision was landed with ongoing or failed builds.Jun 9 2021, 8:21 AM
This revision was automatically updated to reflect the committed changes.

Done. I have run tests locally and they were fine, but please look out for buildbot emails to make sure it didn't break anything we didn't test. Thanks!

thakis added a subscriber: thakis.Jun 9 2021, 9:26 AM

This breaks tests everywhere as far as I can tell. Eg http://45.33.8.238/linux/48494/step_12.txt

Please take a look and revert for now if it takes a while to fix.

Thanks Nico! Sorry, I was out for a while. Not sure why I didn't catch this on my build.

@koutheir how did you test this?

@rengolin I tested the ARM tests that I thought would be affected. This appears to be affecting other tests.

I'm currently running a full check-llvm for the ancestor commit of 64e9aa33020d, after which I'll also perform another check-llvm for the commit 64e9aa33020d. If more tests are failing with the commit 64e9aa33020d, then I'll fix them and update the patch.

@rengolin I tested the ARM tests that I thought would be affected. This appears to be affecting other tests.

Right, before submitting a patch, you should run at the very least "ninja check" on the affected components, in your case, LLVM only.

I'll do that from now on. I'm sorry for the confusion.

koutheir updated this revision to Diff 350995.Jun 9 2021, 2:31 PM

@rengolin: Can you please commit this updated patch on my behalf?

I'm having trouble building and testing, if anyone else could commit this for the time being, I'd appreciate it. Thanks!

@efriedma @joerg

I'm having trouble building and testing, if anyone else could commit this for the time being, I'd appreciate it. Thanks!

Ok, managed to fix my environment, built and tested fine on x86_64. Let me know if bots crash again.

The commit 789708617d20 caused two failures:

However, I believe these failures are unrelated to these changes. It looks like a problem of selecting the required C++ standard for the LLVM C++ source files.

The build builders/53/builds/3020 fails due a time out. I don't think this is an actual failure. Maybe someone could increase the time out value for certain long-running tests.

The build builders/53/builds/3020 fails due a time out. I don't think this is an actual failure. Maybe someone could increase the time out value for certain long-running tests.

You're right, none of these breakages seem related to your patch. Thanks for checking.