Hey @aykevl, could you please confirm that this patch looks okay?

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.

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?

Great patch

@ixoo committed in 0f0be3fb8ddeca4bbcffc7b22319254c360ca24b - thanks for the patch!

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.

Thanks for the patch @rodrigorc, appreciate it!

Committed in

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

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.

Couple of nitpicks, otherwise good to go.

This is good, this is good. Nice work @aykevl, approved.

Added an integration test case

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).

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.

@LemonBoy merged in into master in 1335737ee119b59bc25ee6d1bb200cf3b975e196

@vlastik Merged into master in 1420f4efbe7ca34355b2dd85d396197d92cd5e3f.

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

Nice work @LemonBoy, thanks for the patch.

should help targets such as AVR than use a non-zero program address space.

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.

Nice work, I didn't know of this particular XMEGA quirk.

Nice catch, cheers.

Nice work @vlastik, do you need someone to commit this for you?

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

I'm keen to get at least one more approval before merging this one.

The AVR buildbot has been running successfully for a few days now: http://lab.llvm.org:8011/builders/llvm-avr-linux/builds/63.

LGTM