This is an archive of the discontinued LLVM Phabricator instance.

[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:

  • __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.

Diff Detail

Event Timeline

aykevl created this revision.Apr 22 2020, 12:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 22 2020, 12:17 PM
Herald added subscribers: Restricted Project, mgorny, dylanmckay. · View Herald Transcript

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.

compiler-rt/lib/builtins/lshrsi3.c
24

Prefer BCPL-style // comments to C-style /* */

if (...) {
  // Comment
  ...
} else {
  // Comment
  ...
}
compiler-rt/test/builtins/Unit/lshrsi3_test.c
24

Place { in the end of the previous line.

26

The LLVM coding standard uses indent=2. I know that certain compiler-rt are using 4, but for new files, we probably should stick with indent=2.

aykevl updated this revision to Diff 259616.Apr 23 2020, 10:00 AM
  • formatted test files with clang-format
  • moved shift implementations into common _impl.inc files
  • fixed comment style of shift implementations (// instead of /* ... */)

I believe that addresses all review comments.

MaskRay added inline comments.Apr 23 2020, 10:44 AM
compiler-rt/test/builtins/Unit/ashlsi3_test.c
4

Test files don't need file headers. There are plenty of examples in clang/test/

compiler-rt/test/builtins/Unit/ashrsi3_test.c
4

ditto

aykevl marked an inline comment as done.Apr 23 2020, 1:12 PM
aykevl added inline comments.
compiler-rt/test/builtins/Unit/ashlsi3_test.c
4

All the other compiler-rt builtins tests (compiler-rt/test/builtins/Unit/*_test.c) do have a header. Should I deviate from that convention for new files?

One difference between these tests and the Clang tests is that these tests are in some sense full programs (with main) while Clang tests are usually just small incomplete source files, just testing whether the correct IR is generated.

(This file is a modified version of ashldi3_test.c, hence the style it follows).

@MaskRay ping?

Also, I was wondering about those lint checks. I can apply them, but IMHO they reduce readability.

MaskRay added inline comments.Jun 16 2020, 5:20 PM
compiler-rt/lib/builtins/int_ashl_impl.inc
13 ↗(On Diff #259616)

Use inline. We compile these files with -std=c11 (inline is a C99 keyword)

compiler-rt/test/builtins/Unit/ashlsi3_test.c
4

I think you can deviate from them. They apparently deviate from the common practice in llvm.

REQUIRES: should be place at the top.

34

It may be simpler organizing all tests in an array of arrays.

aykevl updated this revision to Diff 272721.Jun 23 2020, 7:34 AM
  • Changed __inline to inline
  • Put REQUIRES line at the top

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).

luismarques added inline comments.Jun 29 2020, 7:20 AM
compiler-rt/lib/builtins/int_types.h
37–56

Are these macro definitions processor-specific? If so, can't we use something more general?

aykevl marked an inline comment as done.Jul 10 2020, 8:55 AM
aykevl added inline comments.
compiler-rt/lib/builtins/int_types.h
37–56

I'm not sure what YUGA means exactly. The constants are defined in int_endianness.h and are also used in various other places in int_types.h (this file). I'm just following the existing convention.

jwagen added a subscriber: jwagen.Aug 10 2020, 12:31 PM
atrosinenko added a reviewer: atrosinenko.EditedAug 13 2020, 2:47 AM
atrosinenko added a subscriber: atrosinenko.

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)...

compiler-rt/lib/builtins/ashrsi3.c
22
compiler-rt/test/builtins/Unit/ashlsi3_test.c
21
compiler-rt/test/builtins/Unit/ashrsi3_test.c
21
compiler-rt/test/builtins/Unit/lshrsi3_test.c
21

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.

compiler-rt/lib/builtins/int_ashl_impl.inc
14 ↗(On Diff #272721)

As I understand, different widths were mentioned in bits_in_?word for different LibCalls (such as bits_in_dword for __ashlti3). Maybe something more generic such as half_width_bits would fit better.

19 ↗(On Diff #272721)

Then, this could be written as half_width_bits <= b < , hmm, total_bits - not actually declared as a constant, but hopefully would be useful for clarity.

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).

aykevl abandoned this revision.Aug 13 2022, 4:31 PM

Not necessary anymore for AVR so abandoning. Anyone: feel free to use this patch however you see fit.

Herald added a project: Restricted Project. · View Herald TranscriptAug 13 2022, 4:31 PM