Page MenuHomePhabricator

[RFC] Make the default LibCall implementations from compiler-rt builtins library more customizable
Needs ReviewPublic

Authored by atrosinenko on Jul 27 2020, 4:50 AM.

Details

Summary

Background

  • builtins library from compiler-rt has portable default implementations written in C for all its LibCalls
  • those implementations can be replaced by potentially much efficient target-specific ones when appropriate
  • ideally, one should be able to start with all-default implementations ASAP when adding support for a new target, and then gradually improve it

Problem statement

  • some targets have special requirements on LibCalls:
    • name aliases (see existing ARM-specific aliases, MSP430 requires some as well)
    • seemingly arbitrarily chosen calling conventions (see Section 6.3 of MSP430 EABI document). While they are probably justified by implementation specifics, it is hard to deduce the required convention merely from the signature (harder than just "special iff is LibCall and has two 64-bit arguments")
    • the default implementation (that potentially is already quite elaborate) may perfectly fit all the requirements apart from those tiny configuration details
  • it is worth not to force target implementations to diverge from the upstream ones just to account for such tiny details
  • on the other hand, it is worth to not clobber the generic implementation with a bunch of target-specific defines
  • bonus point: still catching as much errors at compile-time as possible

This proposal

This RFC includes an example implementation.

  • + all adjustments listed above (calling conventions, name aliases) will be contained inside a single target specific header and a couple of lines in int_lib.h that conditionally #include that header
    • + the default LibCall implementations will have O(1) customization-related code per file irrespective of how many targets perform some customizations
  • - has large patch size
    • + is by design a one-time refactoring simplifying adding new targets in the future
    • + can already help factoring out the existing ARM-specific name aliases
    • most of the changes was performed automatically by sed, so they (and their corresponding scripts) can be reviewed separately while spending most of the time on manual changes
  • - has a shell script that may need re-running from time to time
    • it is not required to have bash on every system that will just compile LLVM since the resulting header is expected to be committed to the repository
    • there already exist some scripts under compiler-rt/utils that generate some headers for NetBSD
    • - there is a possibility that someday someone will forget to re-run it
    • + most probably, the only cases when running it is actually required are
      • adding some new LibCall
      • changing the set of parameters (now, there exist two: "calling convention" = "misc attributes" and "name aliases" = "misc footer")
    • + it seems to have much simpler requirements on how to run it than some of those NetBSD-related scripts :)
    • - more real-world mistake would probably be to make a simple change (such as adding one another generic source file) and updating the autogenerated header manually because it was so trivial change, so when the script will be eventually re-run, there would be some unexpected changes
      • + because the script has very weak assumptions on how it should be run, we can just have a test (that requires bash) that ensures that the committed script and the committed header match (something like checking code style by auto-formatting and checking that nothing changes)
  • + when something was made wrong, chances are high it would be caught early
    • declaring a new LibCall via DECLARE_LIBCALL macro without updating the script immediately triggers a compilation error
    • - on the target implementation side, there are no safety nets, so one can easily mistype a LibCall name in a statement like #define AUX_DECLS__ashldi3 ... that would be silently ignored in this approach
      • there is a chance this would be caught by target-specific tests and debugged and fixed early by the same person who is implementing that target's support (at least, when changing calling convention)
      • the most care should probably be taken when reviewing this particular initial implementation, as I have to "import" the existing ARM-specific aliases

Diff Detail

Event Timeline

atrosinenko created this revision.Jul 27 2020, 4:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 27 2020, 4:50 AM
Herald added subscribers: Restricted Project, kristof.beyls, krytarowski. · View Herald Transcript
atrosinenko edited the summary of this revision. (Show Details)Jul 27 2020, 4:53 AM
atrosinenko retitled this revision from [WIP] Create infrastructure for customizing the default LibCall implementations to [RFC] Make the default LibCall implementations from compiler-rt builtins library more customizable.
atrosinenko requested review of this revision.Jul 27 2020, 4:57 AM
atrosinenko added reviewers: compnerd, phosek.
atrosinenko edited the summary of this revision. (Show Details)Aug 5 2020, 9:10 AM
aaron.ballman added inline comments.
compiler-rt/lib/builtins/arm-libcall-overrides.h
8

It looks like you're missing a header guard, is that intentional?

Another note to myself: add AUX_DECLS(__LibCallName) to every generic implementation file. They were forgotten unintentionally.

compiler-rt/lib/builtins/arm-libcall-overrides.h
8

Thanks, there are files such as libcall-set-defaults.inc that is explicitly named .inc due to some specific requirements at which point it should be included. But this one definitely lacks a header guard. Another question is whether it lacks a guard to check whether we are really on ARM. But it would probably be better to have something like

#if !defined(__ARM_EABI__)
# error Do not include this header unless on ARM target.
#endif