[AVR] Don't adjust for instruction size

[aykevl]

Summary

I'm not entirely sure why this was ever needed, but when I remove both adjustments all tests still pass.

This fixes a bug where a long branch (using the jmp instead of the rjmp instruction) was incorrectly adjusted by 2 because it jumps to an absolute address instead of a PC-relative address. This is the case in the ctzsi2 test of compiler-rt. I could have added AVR::fixup_call to the list of exceptions, but it seemed more sensible to me to just remove this code: removing it doesn't seem to cause a regression anywhere.

---

Here is a gist which demonstrates the problem: https://gist.github.com/aykevl/d23d91a39589f5b4abc13b3dd0b304c4
Run it using:

clang --target=avr -mmcu=atmega1284p -Os -c -o avr.o avr.c; and avr-objdump -drz avr.o | tail -n 4

Before this patch:

 802:	0e 94 00 00 	call	0	; 0x0 <foo>
			802: R_AVR_CALL	bar
 806:	0d 94 00 00 	jmp	0x20000	; 0x20000 <foo+0x20000>
			806: R_AVR_CALL	.text

After this patch:

 802:	0e 94 00 00 	call	0	; 0x0 <foo>
			802: R_AVR_CALL	bar
 806:	0c 94 00 00 	jmp	0	; 0x0 <foo>
			806: R_AVR_CALL	.text

You may be wondering why the offset is 0x20000 instead of 0x2. It made finding the real cause much harder. It seems to be related to the code here:
https://github.com/llvm/llvm-project/blob/release/10.x/llvm/lib/Target/AVR/MCTargetDesc/AVRAsmBackend.cpp#L101

Look at this line in the comment:

/// Resolves to:
/// 1001 kkkk 010k kkkk kkkk kkkk 111k kkkk

I think this is a misreading of the AVR ISA document, which is also reflected in the code below the comment. It should be 1001 010k kkkk 111k kkkk kkkk kkkk kkkk (row/column swap in the table).
Luckily the input is normally 0, so the output is accidentally correct. I don't know when the input value is ever non-zero. In any case, that's a separate issue that should be fixed separately.

[aykevl]

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.

That patch for branch/rcall/rjmp disassembler support conflicts with this patch so I'm waiting for this one to be reviewed before submitting the disassembler patch.

[dylanmckay]

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

Before merging this, I want to run it under the emulated AVR integration tests I've been working on, just to build 100% confidence that it does what it says on the tin. The tests I've been working on are getting very close to useful now, but it may be a couple more weeks before I get to approving this. I remember writing this particular logic quite some time ago, and it felt nontrivial for some reason I cannot remember.

Keep up the good work, I'll come back to this very soon

[aykevl]

Glad to see you're working on AVR again!

In D78459#2040419, @dylanmckay wrote:

Before merging this, I want to run it under the emulated AVR integration tests I've been working on, just to build 100% confidence that it does what it says on the tin.

That sounds great! As you can see with the patches I submitted, I hit a number of miscompilations when I tried to compile and run the compiler-rt tests. I'm sure there are more. Are you planning on adding these integration tests to the LLVM buildbot or something more manual?

Eventually, we might be able to run the GCC torture test suite to find the remaining bugs. It is a whole lot easier to find a bug in a small contained test than it is to find a bug in a larger application that only breaks on AVR. Even known-working and known-broken tests would be useful, to track progress (and catch regressions soon after they've been introduced).

Anyway, looking forward to these tests.

[dylanmckay]

llvm/lib/Target/AVR/MCTargetDesc/AVRAsmBackend.cpp

92

Copy-paste your avr-jmp.c test into llvm-project/clang/test/CodeGen/avr

https://gist.github.com/aykevl/d23d91a39589f5b4abc13b3dd0b304c4

Add CHECK lines for the new disassembly output

 802:	0e 94 00 00 	call	0	; 0x0 <foo>
			802: R_AVR_CALL	bar
 806:	0c 94 00 00 	jmp	0	; 0x0 <foo>
			806: R_AVR_CALL	.text

I'm going to add your avr-jmp.c test into the integration test suite as well. I intend to set up a build status page for it soon.

Really good patch. You may be right about the misreading of the CALL machine specification - good catch.

[dylanmckay]

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.

https://github.com/dylanmckay/avr-compiler-integration-tests/commit/6c4c67787c3525ec719d4834180874966dbfb2af

I will approve this patch once the avr-jmp.c test is added to the clang test suite. in this patch.

[dylanmckay]

Are you planning on adding these integration tests to the LLVM buildbot or something more manual?

At the moment, it's something more manual, but I'm working on having it automated.

I don't think it is tenable to add the AVR simulator integration tests to the LLVM buildbot, but I haven't asked llvm-dev yet. I remember reading a thread on llvm-dev fairly recently for a similar problem where a developer wanted to add automated checks to all LLVM changes. It was suggested to integrate the tests into Phabricator rather than the buildbot, perhaps this is something AVR can do. In that thread, a Google project that adds some "Pre-merge checks" (also included on this differential) to Phabricator was suggested as a good starting point. There may be considerations due to the progressive move towards LLVM enabling GitHub features on its repositories over time.

Eventually, we might be able to run the GCC torture test suite to find the remaining bugs.

I haven't looked into GCC torture suite, I wasn't aware we could run with non-GNU compilers like Clang but that makes sense. This would be very interesting to run it for AVR.

[aykevl]

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

---

Unrelated: I have written a test system for compiler-rt, did you see it?
https://github.com/aykevl/llvm-project/blob/avr-compiler-rt/compiler-rt/test/builtins/Unit/avr-runner.py
https://github.com/aykevl/llvm-project/blob/avr-compiler-rt/compiler-rt/test/builtins/Unit/avr-runner.c
It's not the cleanest code, but feel free to use it for the integration test. Testing AVR compiler-rt (even if not all the tests pass yet) can be a good way to ensure working builtins won't break.

[dylanmckay]

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

That's fine, I've seen many much larger LLVM unit tests (there's one X86 one that's over 32,000 IIRC). Characters are cheap, certainly good price compared to the bugs they guard against.

Thanks again for the patch @aykevl

It's not the cleanest code, but feel free to use it for the integration test. Testing AVR compiler-rt (even if not all the tests pass yet) can be a good way to ensure working builtins won't break.

That's very neat, plus the code is in-tree, which is even better. It would be nice to add to the integration tests - ideally, we can wire extra tests up easy enough. I will have to a deeper look into it and wire it up, also set up 24/7 automation with the suite itself. I'm looking into writing a Ruby script for LLVM Phabricator (I would be able to merge this patches much much quicker if they can be automatically validated/(read: validated without recompiling LLVM manually)

[dylanmckay]

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.

[aykevl]

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:

00000000 <foo>:
		...
     800: 00 00                        	nop
     802: 0e 94 00 00                  	<unknown>
     806: 0d 94 00 00                  	<unknown>

avr-objdump shows the bug, this is without the patch applied:

00000000 <foo>:
	...
 800:	00 00       	nop
 802:	0e 94 00 00 	call	0	; 0x0 <foo>
 806:	0d 94 00 00 	jmp	0x20000	; 0x20000 <foo+0x20000>

Any idea how to proceed?

In D78459#2091779, @dylanmckay wrote:

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.

Yes I'm aware of bugpoint (I've used it often to reduce test cases). Unfortunately the bug is only triggered on long branches, so I don't think it's possible to avoid it in this case.

[efriedma]

On ARM and AArch64, we have a special intrinsic/pseudo-instruction that intentionally eats as many bytes as you want. See llvm.arm.space/llvm.aarch64.space. This allows constructing a "large" testcase that isn't many lines. Useful for testing stuff like branch relaxation.

[aykevl]

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

@efriedma Ah thanks for the idea! Not sure it's worth it just for this test though.

[aykevl]

• Add testcase

I have added an IR testcase instead of a C testcase as that seemed better to me (closer to what we want to test). Now that D81961 is in, it's actually possible to test this.

llvm/lib/Target/AVR/MCTargetDesc/AVRAsmBackend.cpp

 
 /// Adjusts the value of a relative branch target before fixup application.
 void adjustRelativeBranch(unsigned Size, const MCFixup &Fixup, uint64_t &Value,
                           MCContext *Ctx = nullptr) {
   // We have one extra bit of precision because the value is rightshifted by
   // one.
   signed_width(Size + 1, Value, std::string("branch target"), Fixup, Ctx);
 
-  Value -= 2;
-
   // Rightshifts the value by one.
   AVR::fixups::adjustBranchTarget(Value);
 }
 
 /// 22-bit absolute fixup.
 ///
 /// Resolves to:
 /// 1001 kkkk 010k kkkk kkkk kkkk 111k kkkk

 void AVRAsmBackend::adjustFixupValue(const MCFixup &Fixup,
                                      const MCValue &Target,
                                      uint64_t &Value,
                                      MCContext *Ctx) const {
   // The size of the fixup in bits.
   uint64_t Size = AVRAsmBackend::getFixupKindInfo(Fixup.getKind()).TargetSize;
 
   unsigned Kind = Fixup.getKind();
-
-  // Parsed LLVM-generated temporary labels are already
-  // adjusted for instruction size, but normal labels aren't.
-  //
-  // To handle both cases, we simply un-adjust the temporary label
-  // case so it acts like all other labels.
-  if (const MCSymbolRefExpr *A = Target.getSymA()) {
-    if (A->getSymbol().isTemporary()) {
-      switch (Kind) {
-      case FK_Data_1:
-      case FK_Data_2:
-      case FK_Data_4:
-      case FK_Data_8:
-        // Don't shift value for absolute addresses.
-        break;
-      default:
-        Value += 2;
-      }
-    }
-  }
-
   switch (Kind) {
   default:
     llvm_unreachable("unhandled fixup");
   case AVR::fixup_7_pcrel:
     adjust::fixup_7_pcrel(Size, Fixup, Value, Ctx);
     break;
   case AVR::fixup_13_pcrel:
     adjust::fixup_13_pcrel(Size, Fixup, Value, Ctx);