[PATCH] D78459: [AVR] Don't adjust for instruction size

[Ayke via Phabricator via llvm-commits]

aykevl created this revision.
aykevl added a reviewer: dylanmckay.
Herald added subscribers: Jim, hiraditya.
Herald added a project: LLVM.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D78459

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


Index: llvm/lib/Target/AVR/MCTargetDesc/AVRAsmBackend.cpp
===================================================================
--- llvm/lib/Target/AVR/MCTargetDesc/AVRAsmBackend.cpp
+++ llvm/lib/Target/AVR/MCTargetDesc/AVRAsmBackend.cpp
@@ -89,8 +89,6 @@
   // one.
   signed_width(Size + 1, Value, std::string("branch target"), Fixup, Ctx);
 
-  Value -= 2;
-
   // Rightshifts the value by one.
   AVR::fixups::adjustBranchTarget(Value);
 }
@@ -237,27 +235,6 @@
   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");


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D78459.258617.patch
Type: text/x-patch
Size: 1235 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200419/e2200076/attachment.bin>