- User Since
- Aug 18 2015, 2:21 AM (270 w, 15 m)
Wed, Sep 30
Good bugfix - thanks for the second patch!
The code is looking nice, thanks for the patch @amikhalev!
This is a great patch, thanks @couchand!
Mon, Sep 21
Hey @aykevl, could you please confirm that this patch looks okay?
Aug 27 2020
Note to the readers: A recent-ish llvm-dev discussion around this patch can be found in here whrein a consensus is found on the way forward on this patch.
Aug 26 2020
I've split the backwards compat tests and AMDGPU changes out to D84345. Is the DataLayout upgrade what you meant with bitcode compatibility tests or should I be adding additional tests?
Aug 24 2020
Aug 11 2020
Looks good, thanks for the patch. Do you need someone to commit this for you @ixoo?
Patch is looking really good, only suggestions are the adding a test and a couple formatting nitpicks.
Jun 23 2020
Thanks for the patch @rodrigorc, appreciate it!
Jun 19 2020
Rebased version with @Sh4rK's comments fixed.
@Sh4rK your review is always welcome - I've updated the code with your comments, here's the delta containing just the fixes you've suggested.
This conflicts with master now, he's a rebased patch
Jun 18 2020
Sorry, I lost track of this. Committed in 01741d6dbec11c0a0c8e610f0033831735c78d1e
Note: I don't know whether the way I've made the Z register printable is the correct way. Maybe the register should be added to the MCInst instead.
Good reproduction, works exactly as expected and indeed this value is being miscalculated.
Jun 17 2020
Couple of nitpicks, otherwise good to go.
Jun 16 2020
This is good, this is good. Nice work @aykevl, approved.
Added an integration test case
Jun 14 2020
Also, you may already know about it but if not, the LLVM bugpoint tool can usually heavily reduce testcase size. I usually get 99% cutdown from some of the larger IR files. If the file must be large though, it must be large (as some tests are) so let's add it anyway.
Ok, I can add the test case (need to recompile LLVM first). It just seems a bit large to me (over 1000 lines).
Jun 8 2020
May 31 2020
I've checked over all of the relocation logic, cross referenced with both the AVR ISA manual and the AVR LLVM fixture implementations in AVRAsmBackend.cpp - everything looks consistent to me.
Are you planning on adding these integration tests to the LLVM buildbot or something more manual?
Added an integration test for this and verified that the test does indeed miscompile and crash the AVR simulator before the patch, but work correctly afterwards. The testcase output is also consistent with AVR-GCC.
May 30 2020
May 17 2020
@LemonBoy merged in into master in 1335737ee119b59bc25ee6d1bb200cf3b975e196
@vlastik Merged into master in 1420f4efbe7ca34355b2dd85d396197d92cd5e3f.
May 16 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).
Good question, I wasn't able to find any document online.
All the info I've used come from this ISA manual and by feeding some asm into ld to see if the result matched my expectations.
I really like the idea behind this patch, but I suspect there is a better way to go about it.
Hey @aykevl, I like this patch, it definitely removes a special cases that intuitively feels like it should not need to be there. Nice work
Reposting a comment from Matt from earlier
May 10 2020
Nice work @LemonBoy, thanks for the patch.
Apr 21 2020
should help targets such as AVR than use a non-zero program address space.
Apr 18 2020
I'm not sure that this is a miscompile. Note that AVR has no "add with immediate" instruction, instead subsumed by the "subtract with immediate" instruction, which of course with immediate values being immediately known to the compiler, can be negated directly at the callsite to effectively form a "add with immediate" instruction.
Good catch, great fix.
Really nice work, it's good to go.
Apr 11 2020
Nice work, I didn't know of this particular XMEGA quirk.
Nice catch, cheers.
Apr 1 2020
Mar 31 2020
Nice work @vlastik, do you need someone to commit this for you?
Mar 30 2020
I think we should add a test, it shouldn't be too hard I think.
Nice patch by the way, the code looks fine, if we can get a basic test in here then it's good to go