Page MenuHomePhabricator

Add AVR target and toolchain to Clang
ClosedPublic

Authored by saaadhu on Nov 24 2016, 11:05 PM.

Details

Summary

This patch adds barebones support in Clang for the (experimental) AVR target. It uses the integrated assembler for assembly, and the GNU linker for linking, as lld doesn't know about the target yet.

The DataLayout string is the same as the one in AVRTargetMachine.cpp. The alignment specs look wrong to me, as it's an 8 bit target and all types only need 8 bit alignment. Clang failed with a datalayout mismatch error when I tried to change it, so I left it that way for now.

Diff Detail

Repository
rL LLVM

Event Timeline

saaadhu updated this revision to Diff 79275.Nov 24 2016, 11:05 PM
saaadhu retitled this revision from to Add AVR target and toolchain to Clang.
saaadhu updated this object.
saaadhu added reviewers: arsenm, mcrosier.
saaadhu updated this object.Nov 24 2016, 11:36 PM
saaadhu edited reviewers, added: dylanmckay, rsmith; removed: mcrosier, arsenm.
dylanmckay edited edge metadata.Nov 25 2016, 2:14 PM

I'm quite happy with this (some minor code alignment issues), but I'd definitely like to see someone from the clang project look this over.

I'm not entirely sure if there's any policies regarding enabling clang for experimental backends.

lib/Driver/Driver.cpp
3755 ↗(On Diff #79275)

Incorrect indentation on the break

lib/Driver/ToolChains.cpp
5443 ↗(On Diff #79275)

Alignment

lib/Driver/ToolChains.h
1328 ↗(On Diff #79275)

Alignment messed up here also.

lib/Driver/Tools.cpp
12105 ↗(On Diff #79275)

Alignment

12116 ↗(On Diff #79275)

This will fail linking as it won't link any CRT library.

I have no issue with leaving it out and doing it in a later patch though, got to start somewhere.

CRT support won't be very elegant as there will probably be a table of MCU name -> CRT lib, as there are several CRTs bundled with AVR GCC.

saaadhu updated this revision to Diff 79332.Nov 26 2016, 6:51 AM
saaadhu updated this object.
saaadhu edited edge metadata.

Fixed alignment issues.

saaadhu added inline comments.Nov 26 2016, 6:58 AM
lib/Driver/Tools.cpp
12116 ↗(On Diff #79275)

Yes, the linked image won't run correctly, but the linker (a recent avr-ld) does run and generate a valid ELF.

And yes, the mapping from mcu to the correct lib is going to be ugly.

jroelofs added inline comments.
lib/Basic/Targets.cpp
8376 ↗(On Diff #79332)

All of this ^ needs a testcase. I recommend adding to test/Preprocessor/init.c

saaadhu updated this revision to Diff 80384.Dec 6 2016, 1:22 AM

Add testcases to test/Preprocessor/init.c
Correct types for WChar and WInt

The ABI is documented in the avr-gcc wiki (https://gcc.gnu.org/wiki/avr-gcc)
I took the output of a trunk build of avr-gcc -dM -E and put that in the Preprocessor/init.c testcase, removing everything clang does not define. The conflicting defines were

  1. DBL and LDBL related defines - clang appears to consider doubles as 64 bit even if DoubleWidth is 32 bit
  2. SIG_ATOMIC defines.

Add testcases to test/Preprocessor/init.c

Awesome, thanks!

Correct types for WChar and WInt

The ABI is documented in the avr-gcc wiki (https://gcc.gnu.org/wiki/avr-gcc)
I took the output of a trunk build of avr-gcc -dM -E and put that in the Preprocessor/init.c testcase, removing everything clang does not define.

Good plan.

The conflicting defines were

  1. DBL and LDBL related defines - clang appears to consider doubles as 64 bit even if DoubleWidth is 32 bit

I /think/ you need to set LongDoubleFormat for that to work.

  1. SIG_ATOMIC defines.

What are the differences there?

saaadhu updated this revision to Diff 80575.Dec 7 2016, 5:00 AM

Thanks, setting DoubleFormat and LongDoubleFormat fixed the DBL_ and LDBL_ differences. Also, setting SigAtomicType fixed the __SIG_ATOMIC_ differences as well. I've added those defines to the test. Unrelated, but I also removed a redundant set of LongLongAlign and overriding of GetDefaultDwarfVersion (returning 2 wasn't necessary).

The only remaining (textually) conflicting defines between "avr-gcc -dM -E" and "clang -dM -E" are

  1. short vs int for a bunch of xxx_TYPE defines, like say CHAR16_TYPE. short and int are both 2 bytes for the avr, so I think either one is ok.
  2. Casts vs plain literals in the defines for double MIN, MAX values. avr-gcc, for example, defines #define DBL_MAX ((double)3.40282347e+38L), whereas clang does #define DBL_MAX 3.40282347e+38. Again, I think this shouldn't matter - I've added the literals sans the cast to the test.

Thanks, setting DoubleFormat and LongDoubleFormat fixed the DBL_ and LDBL_ differences. Also, setting SigAtomicType fixed the __SIG_ATOMIC_ differences as well. I've added those defines to the test. Unrelated, but I also removed a redundant set of LongLongAlign and overriding of GetDefaultDwarfVersion (returning 2 wasn't necessary).

The only remaining (textually) conflicting defines between "avr-gcc -dM -E" and "clang -dM -E" are

  1. short vs int for a bunch of xxx_TYPE defines, like say CHAR16_TYPE. short and int are both 2 bytes for the avr, so I think either one is ok.

This will bite you when it comes to C++. Those two types are mangled differently, so you'll get an ABI mismatch in overloads that use those typedefs.

  1. Casts vs plain literals in the defines for double MIN, MAX values. avr-gcc, for example, defines #define DBL_MAX ((double)3.40282347e+38L), whereas clang does #define DBL_MAX 3.40282347e+38. Again, I think this shouldn't matter - I've added the literals sans the cast to the test.

That should be fine.

saaadhu updated this revision to Diff 80721.Dec 7 2016, 11:16 PM

Make defines for CHAR16_TYPE, {U,}INT_{LEAST,FAST}16_TYPE use int instead of short.

{U,}INT16_TYPE still gets defined as short though - lib/Frontend/InitPreprocessor.cpp::DefineExactWidthIntType does not use TargetInfo::getIntTypeByWidth. Instead, InitializePredefinedMacros calls the function with the specific type (SignedShort/UnsignedShort), as getShortWidth() > getCharWidth(), but getIntWidth() == getShortWidth(). Not sure what the best way to fix that is - should I make DefineExactWidthType use TargetInfo::getIntTypeByWidth?

Make defines for CHAR16_TYPE, {U,}INT_{LEAST,FAST}16_TYPE use int instead of short.

{U,}INT16_TYPE still gets defined as short though - lib/Frontend/InitPreprocessor.cpp::DefineExactWidthIntType does not use TargetInfo::getIntTypeByWidth. Instead, InitializePredefinedMacros calls the function with the specific type (SignedShort/UnsignedShort), as getShortWidth() > getCharWidth(), but getIntWidth() == getShortWidth(). Not sure what the best way to fix that is - should I make DefineExactWidthType use TargetInfo::getIntTypeByWidth?

I'm not sure either. I think it's a good question for @rengolin.

dylanmckay added inline comments.Dec 10 2016, 3:04 AM
lib/Basic/Targets.cpp
8543 ↗(On Diff #79275)

If we build clang without LLVM_EXPERIMENTAL_TARGETS_TO_BUILD=AVR, how will this work?

Will it fail compilation? Will clang report that AVR is supported? Will it crash if you try and run clang?

rengolin edited edge metadata.Dec 14 2016, 5:03 AM

Make defines for CHAR16_TYPE, {U,}INT_{LEAST,FAST}16_TYPE use int instead of short.

{U,}INT16_TYPE still gets defined as short though - lib/Frontend/InitPreprocessor.cpp::DefineExactWidthIntType does not use TargetInfo::getIntTypeByWidth. Instead, InitializePredefinedMacros calls the function with the specific type (SignedShort/UnsignedShort), as getShortWidth() > getCharWidth(), but getIntWidth() == getShortWidth(). Not sure what the best way to fix that is - should I make DefineExactWidthType use TargetInfo::getIntTypeByWidth?

I'm not sure either. I think it's a good question for @rengolin.

Hum, sorry, can't answer that one. :)

I find it confusing that you define CHAR16 as int and INT16 as short, with whatever INT_FAST16 is as int.

I don't know much about AVR, but the C standard predicts that short and int can have the same size, so Clang should be able to cope with it.

cheers,
--renato

lib/Basic/Targets.cpp
8543 ↗(On Diff #79275)

AFAIK, Clang support for targets is independent of LLVM. A standard Clang build supports all targets up to LLVM IR (so all ABIs, PCSs, type descriptions, etc.), but it will fail if the back-end is not available and Clang is invoked beyond IR.

This is why we can only have IR tests in Clang. As long as all your tests stop at IR, this should be fine.

Make defines for CHAR16_TYPE, {U,}INT_{LEAST,FAST}16_TYPE use int instead of short.

{U,}INT16_TYPE still gets defined as short though - lib/Frontend/InitPreprocessor.cpp::DefineExactWidthIntType does not use TargetInfo::getIntTypeByWidth. Instead, InitializePredefinedMacros calls the function with the specific type (SignedShort/UnsignedShort), as getShortWidth() > getCharWidth(), but getIntWidth() == getShortWidth(). Not sure what the best way to fix that is - should I make DefineExactWidthType use TargetInfo::getIntTypeByWidth?

If you do, it might break other things. Might be better to leave this alone, and leave a comment with a PR for it, explaining where the differences are.

I'm not sure either. I think it's a good question for @rengolin.

Would someone with a llvm bugzilla account please file the PR for me? New user registration is disabled, and I've been waiting for llvm-admin@lists.llvm.org to respond for nearly a week now.

And how should I proceed after that? Would one of the reviewers commit the patch for me?

Signed off by Jonathan Roelofs via cfe-commits

This revision was automatically updated to reflect the committed changes.