- User Since
- Apr 25 2018, 9:12 AM (125 w, 4 d)
Mon, Sep 14
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.
Sun, Aug 23
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).
Aug 9 2020
Aug 7 2020
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.
Jul 25 2020
Jul 12 2020
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.
Jul 11 2020
I think this addresses all review comments.
- add relocation types in comments
Jul 10 2020
Thanks for the review! This should address all comments.
Jul 9 2020
Jul 1 2020
- 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...
Jun 30 2020
@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.
Jun 29 2020
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:
Jun 26 2020
Jun 25 2020
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.
Jun 23 2020
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
Jun 22 2020
Jun 19 2020
Jun 18 2020
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.
- fix formatting
- updated formatting
- Add testcase
Jun 16 2020
I have made a patch to support disassembly of jmp/call instructions, which should make this patch testable: D81961
Jun 14 2020
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:
Jun 10 2020
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?
Jun 9 2020
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.
May 18 2020
Glad to see you're working on AVR again!
See my inline comment.
May 7 2020
Apr 29 2020
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).
Apr 28 2020
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.
Apr 26 2020
- better formatting of return statement
- added testcases to exercise all the bits in every instruction
Apr 25 2020
Apr 23 2020
- 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