This patch adds support for the following 32-bit shift builtins:
- __ashlsi3
- __ashrsi3
- __lshrsi3
They are needed on architectures that do not have a 32-bit shift instruction, such as AVR.
Note: this patch depends on D78662.
Paths
| Differential D78663
[builtins] Add 32-bit shift builtins AbandonedPublic Authored by aykevl on Apr 22 2020, 12:17 PM.
Details Summary This patch adds support for the following 32-bit shift builtins:
They are needed on architectures that do not have a 32-bit shift instruction, such as AVR. Note: this patch depends on D78662.
Diff Detail
Event TimelineComment Actions Shall we factor out the common part of lshrti3.c and lshrdi3.c and place that in an .inc file? We can thus avoid duplication.
Comment Actions
I believe that addresses all review comments. aykevl added inline comments.
Comment Actions @MaskRay ping? Also, I was wondering about those lint checks. I can apply them, but IMHO they reduce readability.
Comment Actions
I'm not so sure about moving to an array of arrays. Coming from a Go background I'm used to this pattern (called "table driven tests") and I normally prefer it, but this pattern is rarely used in compiler-rt builtin tests and in particular is not used in the 64-bit versions of these tests (from which the 32-bit tests were derived: they are essentially the same tests but with shortened inputs).
Comment Actions A bit of comment fine-tuning. :) UPD: Hmm, trivial "suggested changes" without any comment attached are rendered as a completely empty entries here (they are shown OK inline)...
Comment Actions Thanks @aykevl! This could be useful for MSP430 as well. Especially, the tests would be worth reusing even for target-specific assembly re-implementation of these routines. Another bit of "non functional" comments.
Comment Actions I have fixed this in a different way for AVR, by expanding these instructions inline (just like avr-gcc does): D96677. Therefore, I don't plan on working on this change anymore. @atrosinenko if you want to continue working on this patch (and fix the various issues you found), feel free to take over the patch. You can do this by commandeering it (Add Action -> Commandeer Revision). Comment Actions Not necessary anymore for AVR so abandoning. Anyone: feel free to use this patch however you see fit.
Revision Contents
Diff 272721 compiler-rt/lib/builtins/CMakeLists.txt
compiler-rt/lib/builtins/ashldi3.ccompiler-rt/lib/builtins/ashlsi3.c
compiler-rt/lib/builtins/ashlti3.ccompiler-rt/lib/builtins/ashrdi3.ccompiler-rt/lib/builtins/ashrsi3.c
compiler-rt/lib/builtins/ashrti3.ccompiler-rt/lib/builtins/int_ashl_impl.inc
compiler-rt/lib/builtins/int_ashr_impl.inc
compiler-rt/lib/builtins/int_lshr_impl.inc
compiler-rt/lib/builtins/int_types.h
compiler-rt/lib/builtins/lshrdi3.ccompiler-rt/lib/builtins/lshrsi3.c
compiler-rt/lib/builtins/lshrti3.ccompiler-rt/test/builtins/Unit/ashlsi3_test.c
compiler-rt/test/builtins/Unit/ashrsi3_test.c
compiler-rt/test/builtins/Unit/lshrsi3_test.c
|
clang-format: please reformat the code