Looks reasonable to me, although I can't really comment on the contents of this as I'm not very familiar with this code.

As a quick review (without looking into this carefully): you need to add tests. Without tests this will not be accepted. It also gives a better idea of what it does exactly and thus makes reviewing easier.

I have tested this locally with my compiler-rt test setup. All tests still pass with this patch applied while the binary size is slightly reduced (when optimizing for size).

ping?

That's unfortunate. I was looking forward to this to use in TinyGo. For TinyGo it doesn't matter what the exact behavior is, just that it is something sensible (and not UB). Saturating float operations would be great.

In D82536#2124348, @efriedma wrote:

The way avr-gcc works, the compiler is responsible for figuring this out, not the assembler. A code pointer should be emitted as .word gs(func) i.e. VK_AVR_GS.

Thanks. I'll push this to unblock D78741.

• added --print-imm-hex (so this patch depends on D83634)
• deleted some redundant parentheses
• changed all comment markers to ;

Looks good to me. I've confirmed this works with D78741 and I have updated the patch to make use of it.

I think this addresses all review comments.

• add relocation types in comments

Thanks for the review! This should address all comments.

ping?

• some minor formatting fixes to CHECK lines

I've taken over the old revision, so this is now a duplicate.

@MaskRay thanks! I wasn't aware of that option.

Ok, let's see whether this works...

@efriedma yeah I know, it's not right in the current state. See my previous comment. Do you have any idea how it is possible to get the object type (function or data) when determining the relocation type, when the referenced symbol is external?

Looks like I'm not able to upload a new diff, so I've made a new revision: D82854.

I experimented a bit more but I really can't find a way to tell the difference between undefined functions and undefined globals. Assuming one or the other will still be incorrect in some cases.

I have attached a patch that addresses review comments:

Look reasonable from my POV, but I don't know enough about CMake or the compiler-rt build system to LGTM this.

I could perhaps add a test to relocations-abs.s that .type foo,@object creates a relocation of type R_AVR_16?

• fixed MC test failure

There is a test failure in llvm/test/MC/AVR/relocations-abs.s. With this change the LLVM behavior would start to differ from the avr-gcc behavior. But actually, the avr-gcc behavior might be a bug.

The reason to not call into LLVM directly is because I want to use the compiler driver, to be compatible with all the compiler flags. Reimplementing the assembler driver would be a pain.

(sorry, I missed your comment)

• Changed __inline to inline
• Put REQUIRES line at the top

In D81285#2102948, @atrosinenko wrote:

On renaming fixed width integer types to their traditional names: I would prefer sending such patch afterwards, it would probably be as simple as just running sed --in-place several times.

I suspect the patterns are technically a bug, but may not show up in practice if they're all buggy in the exact same way. If all additions in a chain are replaced with subtractions, then it should work.

Patch committed!

• fix formatting

• updated formatting

• Add testcase

@MaskRay ping?

I have made a patch to support disassembly of jmp/call instructions, which should make this patch testable: D81961

I'm trying to make a test case, but I'm not sure how to do it. I have converted the test to IR here: https://gist.github.com/aykevl/08e87000370d0f7ed7780932b32d7924
The difference in disassembly output is only visible when disassembling the instructions (not just the instruction encodings), and llvm-objdump doesn't yet support 32-bit AVR instructions. Therefore the output is the same with or without this patch:

I need to recompile LLVM to test this patch so it might take a while.

Ok, I can add the test case (need to recompile LLVM first). It just seems a bit large to me (over 1000 lines).

I actually wrote a patch very similar to this one (but not implementing as many relocations). It's here: https://gist.github.com/aykevl/ce3c04e1175d9602c9dfaf0cf91298ba
I wanted to hold off on submitting it before the AVR disassembler was a bit more reliable, to make proper tests.

@echristo any chance you could take a look at this?

I'm not sure whether native_int is any clearer than just int. I'm afraid it only introduces more complexity ("What's native_int? Oh, it's just int").

I tested this patch against my local testing system to make sure it didn't break anything, and I get the same number of failures (most of which are due to a missing complex.h file, which is unrelated). So it looks fine from my point of view, although I know not enough about this code to LGTM it.
Note that double float (float128) doesn't work in my setup, so I can't test changes related to that.

Glad to see you're working on AVR again!

See my inline comment.

In D78662#2006405, @efriedma wrote:

I was trying with msp430; I had trouble getting a testcase to build with AVR (I forget why). I guess AVR actually behaves differently: it doesn't respect "zeroext" markings on arguments, unlike most targets.

Given that, there's probably a bug we need to fix, in either the AVR backend or type legalization.

A thought: should the AVR family be included in the target triple? Like avr5-unknown-unknown (similar to armv6m-none-eabi, armv7m-none-eabi, etc).
The different variations do sometimes change the architecture in incompatible ways, such as with the call instruction which pushes a return address of two or three bytes depending on the architecture variant (and thus makes it impossible to pass parameters on the stack reliably).

While creating a commit for disassembling branch/rcall/rjmp instructions, I noticed that it makes the LLVM assembler for AVR less buggy. Previously it would output the wrong addend for relocations (off by two) and sometimes emit an instruction that was also off by two. This patch fixes that partially: the instructions are correct while the relocations are still off-by-two. So that looks like an improvement.

• better formatting of return statement
• added testcases to exercise all the bits in every instruction

In D78662#2000428, @efriedma wrote:

In summary: I think using int is fine as LLVM internally passes an i8

That doesn't seem quite right. Consider, for example, long long a(long long a, int b) { return a << (char)(b); }. To generate correct code, you need to clear the high bits of b. If we just model the argument as an anyext i8, we'll skip clearing those bits, and the compiler-rt C implementations will have undefined behavior. It looks like LLVM does actually extend the value to "int", though. (It doesn't choose between sign/zero extend consistently, but it doesn't actually matter here).

• moved test files out of the AVR-specific directory
• removed the REQUIRES: line

• formatted test files with clang-format
• moved shift implementations into common _impl.inc files
• fixed comment style of shift implementations (// instead of /* ... */)

• defined two macros clzsi and ctzsi for counting bits in si_int and su_int
• changed int32_t to si_int in two places

In D78662#1997884, @efriedma wrote:

Is making all shift amounts "int" consistent with what LLVM does?