Page MenuHomePhabricator

[compiler-rt][builtins] Add AVR __udivmodsi4 and __divmodsi4
Needs ReviewPublic

Authored by aykevl on Nov 16 2022, 5:22 PM.

Details

Summary

This is the 32-bit division operation for AVR (both unsigned and signed). The function works, but could probably be optimized further somewhat. It will certainly be a lot more efficient than a C version, but most importantly it matches the ABI expected by avr-gcc and LLVM (unlike the compiler-rt C version).

Tests: https://github.com/benshi001/avr-test/pull/2

Also see: https://github.com/llvm/llvm-project/issues/55279

Diff Detail

Event Timeline

aykevl created this revision.Nov 16 2022, 5:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 16 2022, 5:22 PM
Herald added subscribers: Enna1, Jim, dberris. · View Herald Transcript
aykevl requested review of this revision.Nov 16 2022, 5:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 16 2022, 5:22 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
aykevl edited the summary of this revision. (Show Details)Nov 16 2022, 5:25 PM
benshi001 accepted this revision.Nov 16 2022, 10:20 PM

LGTM. Thanks!

This revision is now accepted and ready to land.Nov 16 2022, 10:20 PM
This revision now requires review to proceed.Nov 16 2022, 11:48 PM
compnerd requested changes to this revision.Nov 17 2022, 3:27 PM
compnerd added inline comments.
compiler-rt/lib/builtins/avr/divmodsi4.S
19

Does it make sense to do something like this instead:

#if defined(__AVR_TINY__)
#error "avrtiny is not yet supported"
#endif

This would ensure that someone building for the environment is not greeted with a missing function.

21–29

Please use DEFINE_COMPILER_RT_FUNCTION from assembly.h.

67–70

Please use SYMBOL_NAME from assembly.h.

compiler-rt/lib/builtins/avr/udivmodsi4.S
48

Similar to the other file.

This revision now requires changes to proceed.Nov 17 2022, 3:27 PM
aykevl added inline comments.Nov 22 2022, 5:18 PM
compiler-rt/lib/builtins/avr/divmodsi4.S
19

I don't think so, because then you can't use compiler-rt anymore for these chips (while currently you can). That would be a regression. A future patch can always add support for these little chips.

For context, avrtiny chips often have just 1kB of flash for program storage (with the largest chips having 4kB). That's 512 instructions. Code will likely be so trivial and/or optimized that you're probably not going to need a 32-bit division anyway.

21–29

I would like to agree... but unfortunately there is an issue. The macro uses an assembly separator string. For most architectures, this is %% but for avr-gcc this is $. For Clang this is currently %% but that's a bug, we should match avr-gcc here (fixed in D138535).
If that patch is merged, we could start to use $ in the macro, but that would break compatibility with older Clang versions. I would like to support compiler-rt for AVR for 1-2 LLVM versions at least, so I would prefer not to use this macro for now but wait until the $ separator has been supported in LLVM for a while.

Are you okay with that?

67–70

I guess this doesn't make much sense without DEFINE_COMPILERRT_FUNCTION? (See above).

aykevl requested review of this revision.Dec 7 2022, 4:18 PM

@compnerd can you take a look at my replies?

compnerd added inline comments.Dec 8 2022, 10:38 AM
compiler-rt/lib/builtins/avr/divmodsi4.S
19

Right, I realize that it would mean that you cannot build the library for that target, but that seems better than finding that there are missing symbols there. If we want to continue to support that chip as a build platform, we should just filter the source file out in the build system.

21–29

Why could we not accomodate in assembly.h? We can do a check for __clang__ and even change it based on the clang version. That way we can continue to support them backwards and forwards.

aykevl added inline comments.Dec 30 2022, 3:53 PM
compiler-rt/lib/builtins/avr/divmodsi4.S
19

but that seems better than finding that there are missing symbols there

There are missing symbols right now: that's what this patch is trying to fix. This patch as-is is a strict improvement over what we already have. What this patch does is add __divmodsi4 and __udivmodsi4 to non-avrtiny chips, and leave the avrtiny chips as they already are.
I don't really see your problem with it.

Well, actually, it's a little bit more complicated. There exists the generic divmod implementations but they have a different ABI and so don't work on any AVR chips. So technically this patch will change from miscompiling code on avrtiny to causing a linker error, which IMHO is still an improvement. In summary:

  • this patch changes avrtiny chips (which is an uncommon AVR variant) from miscompiling 32-bit divmod operations to a linker error - if they do a 32-bit divmod at all (which is unlikely considering the size of these chips)
  • this patch changes non-avrtiny chips (the majority) from miscompiling to correctly compiling divmod operations.

So yeah this is not complete, but is an improvement and I hope to add __divmodsi4 etc support for avrtiny in a future patch.

21–29

Why could we not accomodate in assembly.h? We can do a check for __clang__ and even change it based on the clang version. That way we can continue to support them backwards and forwards.

I tried various separators (%%, %, $) but they don't work under LLVM 14. For example, for %% (which in theory would have worked), I get the following error message:

[...]/llvm-project.main/compiler-rt/lib/builtins/avr/divmodhi4.S:29:2: error: unexpected token at start of statement
 %% .globl __divmodhi4 %% .type __divmodhi4,@function %% __divmodhi4:

If you know of a way to still support LLVM 14 then please let me know. Otherwise, I think the only reasonable way forward is to not use DEFINE_COMPILERRT_FUNCTION for now.