[AVR] Override ParseDirective

[xiangzhai]

Summary

Hi LLVM developers,

Motivation:

• avr-ld supports directive, relax.s testcase
• arm-linux-gnu-ld supports that too, tls-gdlerelax.s testcase

But:

$ llvm-mc -filetype=obj -triple=avr-unknown-linux -mcpu=atmega328p relax.s -o relax.o
relax.s:12:2: error: unknown directive
        .word   L2 - L1
        ^

So I simply override ParseDirective for llvm/lib/Target/AVR/AsmParser/AVRAsmParser.cpp, please review it, thanks a lot!

Regards,
Leslie Zhai

[xiangzhai]

Dear Dylan,

BTW: AVR GNU toolchain can *not* support ldi r16, -lo8(abc) but ldi r16, lo8(-(abc)) instead,

$ avr-gcc -mmcu=atmega328p -o avr-gcc-relocations.o -c relocations.s

relocations.s: Assembler messages:
relocations.s:36: Error: garbage at end of line
relocations.s:39: Error: garbage at end of line
relocations.s:45: Error: garbage at end of line
relocations.s:51: Error: garbage at end of line
relocations.s:57: Error: garbage at end of line
relocations.s:69: Error: garbage at end of line
relocations.s:72: Error: garbage at end of line
relocations.s:75: Error: garbage at end of line
relocations.s:36: Error: can't resolve `0' {*UND* section} - `lo8' {*UND* section}
relocations.s:36: Error: expression too complex
relocations.s:39: Error: can't resolve `0' {*UND* section} - `hi8' {*UND* section}
relocations.s:39: Error: expression too complex
relocations.s:45: Error: can't resolve `0' {*UND* section} - `hh8' {*UND* section}
relocations.s:45: Error: expression too complex
relocations.s:51: Error: can't resolve `0' {*UND* section} - `hlo8' {*UND* section}
relocations.s:51: Error: expression too complex
relocations.s:57: Error: can't resolve `0' {*UND* section} - `hhi8' {*UND* section}
relocations.s:57: Error: expression too complex
relocations.s:69: Error: can't resolve `0' {*UND* section} - `pm_lo8' {*UND* section}
relocations.s:69: Error: expression too complex
relocations.s:72: Error: can't resolve `0' {*UND* section} - `pm_hi8' {*UND* section}
relocations.s:72: Error: expression too complex
relocations.s:75: Error: can't resolve `0' {*UND* section} - `pm_hh8' {*UND* section}
relocations.s:75: Error: expression too complex

So do I need to rewrite Check for sign for LLD to compatible GNU toolchain?

Regards,
Leslie Zhai

[xiangzhai]

Add R_AVR_DIFF8, R_AVR_DIFF16 and R_AVR_DIFF32 testcase:

$ avr-readelf -r avr-relax.o 

Relocation section '.rela.text' at offset 0xe8 contains 1 entries:
 Offset     Info    Type            Sym.Value  Sym. Name + Addend
00000000  00000212 R_AVR_CALL        00000000   .text + 0

Relocation section '.rela.data' at offset 0xf4 contains 3 entries:
 Offset     Info    Type            Sym.Value  Sym. Name + Addend
00000000  0000021e R_AVR_DIFF8       00000000   .text + 4
00000001  0000021f R_AVR_DIFF16      00000000   .text + 4
00000003  00000220 R_AVR_DIFF32      00000000   .text + 4

[xiangzhai]

Implement Parse R_AVR_LO8_LDI_NEG and other R_AVR_XXX_NEG Expression for GCC compatible.

Because AVR GNU toolchain only support such Expression:

ldi r16, lo8(-(abc))

But LLVM AVR prefers:

ldi r16, -lo8(abc)

So I just re-implement Check for sign, please review my patch, thanks a lot!

Regards,
Leslie Zhai

[xiangzhai]

Implement R_AVR_LO8_LDI_GS and R_AVR_HI8_LDI_GS:

$ llvm/build/bin/llvm-mc -filetype=obj -triple=avr-unknown-linux -mcpu=atmega328p relocations.s -o avr-llvm-relocations.o
$ avr-readelf -r avr-llvm-relocations.o

...
00000038  00000b18 R_AVR_LO8_LDI_GS  00000000   foo + 0
0000003a  00000b19 R_AVR_HI8_LDI_GS  00000000   foo + 0
...

[dylanmckay]

include/llvm/BinaryFormat/ELFRelocs/AVR.def

41 ↗

(On Diff #116497)

These integers correspond to numeric ELF relocation codes.

Looking at binutils-gdb, the integers for R_AVR_DIFF{8,16,32} are 30, 31, and 32 respectively.

This is concerning because 30 and 31 are already occupied by R_AVR_SYM_DIFF and R_AVR_16_LDST in master.

It looks like the AVR backend doesn't actually generate fixups for either of these, so this will have never caused any problems in the past.

If you rename SYM_DIFF to DIFF8, and then change R_AVR_16_LDST to 33, and then change your new constants to their correct values, it should be fine. I can fix up the relocations in the future.

[xiangzhai]

Change RELOC_NUMBER for AVR Relocation DIFF Types.

[xiangzhai]

Implement R_AVR_16_PM:

relocations.s testcase:

...
.short foo
.short gs(foo)
...

$ avr-readelf -r avr-reloc.o 

Relocation section '.rela.text' at offset 0xa8 contains 4 entries:
 Offset     Info    Type            Sym.Value  Sym. Name + Addend
...
00000002  00000504 R_AVR_16          00000000   foo + 0
00000004  00000505 R_AVR_16_PM       00000000   foo + 0
...

[xiangzhai]

Does it need to Refactory ParseDirective for adding parameter OperandVector Operands to support Modifier?

[xiangzhai]

Waiting for D38304 about Refactoring ParseDirective.

[dylanmckay]

This patch is looking really nice by the way.

lib/Target/AVR/AsmParser/AVRAsmParser.cpp

437

Can we hoist the "gs" into a static constant named `GENERATE_ at the top of the file?

It looks like gs stands for "generate stubs"

https://lists.nongnu.org/archive/html/avr-gcc-list/2010-05/msg00028.html

593

Can we do something like this at the top of the file

cpp
const int SIZE_LONG = 8;
const int SIZE_WORD = 4;

By the way, I think these sizes are for a 32-bit machine. On AVR, long is 5 bytes and int (which I think corresponds to .word) is 2 bytes

Here is a document detailing the AVR integer types and their sizes

603

Can we be more specific and name Size -> SizeInBytes

623

Replace this with

cpp
FIXME: does not support .byte lo8(foo)

Could you also add a bit more information to the comment, because it's not clear why it isn't supported.

[dylanmckay]

lib/Target/AVR/MCTargetDesc/AVRMCExpr.cpp

125 ↗

(On Diff #116754)

I believe we should mask out all of the upper bits here.

It looks like none of the other labels in this method do it either, which looks like a bug to me. I have filed PR34755 to fix these existing ones up.

For these new expressions, can you please mask out the irrelevant bits after shifting?

173 ↗

(On Diff #116754)

Related to an earlier comment; I don't think GS directly maps to program memory

[xiangzhai]

Dear Dylan,

I am not available until October 10th https://mail.kde.org/pipermail/k3b/2017-September/001783.html Happy Chinese National Day!

Regards,
Leslie Zhai

[xiangzhai]

Dear Dylan,

I updated my patch as your suggestion, please review it, thanks a lot!

PS: still waiting for D38304 about Refactoring ParseDirective.

Regards,
Leslie Zhai

[xiangzhai]

Supported .byte lo8(foo) now :)

$ llvm/build/bin/llvm-mc -filetype=obj -triple=avr-unknown-linux -mcpu=atmega328p relocations.s -o avr-llvm-relocations.o
$ avr-readelf -r avr-llvm-relocations.o 

Relocation section '.rela.text' at offset 0x170 contains 60 entries:
 Offset     Info    Type            Sym.Value  Sym. Name + Addend
...
00000071  00000b1b R_AVR_8_LO8       00000000   foo + 0
...

[xiangzhai]

Refactory EmitValue for AVRMCELFStreamer.

[xiangzhai]

Support R_AVR_DIFF8, R_AVR_DIFF16 and R_AVR_DIFF32 now :)

$ llvm-mc -filetype=obj -triple=avr-unknown-linux -mcpu=atmega328p relax.s -o avr-relax-llvm.o

$ avr-readelf -r avr-relax-llvm.o 

Relocation section '.rela.text' at offset 0xd8 contains 1 entries:
 Offset     Info    Type            Sym.Value  Sym. Name + Addend
00000000  00000512 R_AVR_CALL        00000000   .text + 0

Relocation section '.rela.data' at offset 0xe4 contains 3 entries:
 Offset     Info    Type            Sym.Value  Sym. Name + Addend
00000000  0000051e R_AVR_DIFF8       00000000   .text + 0
00000002  0000051f R_AVR_DIFF16      00000000   .text + 0
00000006  00000520 R_AVR_DIFF32      00000000   .text + 0

[arsenm]

lib/Target/AVR/AsmParser/AVRAsmParser.cpp

35

Generally I don't think we use these kinds of hardcoded macro options

[xiangzhai]

Remove hardcoded macro REFACTORY_PARSEDIRECTIVE.

[xiangzhai]

Rebase for rL315531

[xiangzhai]

Rebase for rL316076 and nicely Ping Dylan and Matt :)

[dylanmckay]

Hey Leslie,

I don't have much time to review this in the next 3 weeks, and I also think that D38304 may take even longer to land (which this patch depends on) because it looks to have gone somewhat stale.

[xiangzhai]

Dear Dylan,

Never mind, you are busy for university class :) D38304 is optional dependent, only depends on it when built with -DREFACTORY_PARSEDIRECTIVE.

Regards,
Leslie Zhai

[xiangzhai]

Nicely ping Dylan :)

[xiangzhai]

Ping

[dylanmckay]

Have finished an initial review - change looks pretty good, it should be good to go once my comments are fixed

Sorry for taking so long to review @xiangzhai!

lib/Target/AVR/AsmParser/AVRAsmParser.cpp

14

These headers should be in alphabetical order

58

Before this can be committed, we will need to remove these REFACTORY_PARSEDIRECTIVE guards.

Once this patch has been comitted, D38304 should then be rebased

lib/Target/AVR/MCTargetDesc/AVRAsmBackend.cpp

270 ↗

(On Diff #120182)

gs stands for generate stubs - I don't think we want to apply the program memory adjustment for these. As far as I understand it, AVR::fixuo_lo8_ldi_gs is a load from RAM.

277 ↗

(On Diff #120182)

Same here regarding gs

lib/Target/AVR/MCTargetDesc/AVRELFObjectWriter.cpp

19 ↗

(On Diff #120182)

Place this in alphabetical order

lib/Target/AVR/MCTargetDesc/AVRMCELFStreamer.h

1 ↗

(On Diff #120182)

s/MCElfStreamer/MCELFStreamer

13 ↗

(On Diff #120182)

Header files should be in alphabetical order

39 ↗

(On Diff #120182)

Indent this constructor list like the one above

42 ↗

(On Diff #120182)

Pop an override qualifier on this function

lib/Target/AVR/MCTargetDesc/AVRMCExpr.cpp

137 ↗

(On Diff #120182)

We want to use a different mask for each expression type

For example

cpp
switch (Kind) {
case AVRMCExpr::VK_AVR_LO8_GS:
    Value &= 0xff;
    Value >>= 1;
    break;
  case AVRMCExpr::VK_AVR_HI8_GS:
    Value &= 0xff00;
    Value >>= 9;
    break;
  case AVRMCExpr::VK_AVR_GS:
    Value >>= 1;
    break;

Different variants require masking out different bitsets - lo8 should isolate only the lower 8-bits, hi8 should to the upper.

lib/Target/AVR/MCTargetDesc/AVRMCTargetDesc.cpp

18 ↗

(On Diff #120182)

Alphabetical order

26 ↗

(On Diff #120182)

Alphabetical order

lib/Target/AVR/MCTargetDesc/CMakeLists.txt

10 ↗

(On Diff #120182)

Add this in alphabetical order

test/MC/AVR/relocations.s

81

Nice test

[xiangzhai]

Dear Dylan,

Thanks for your review! I updated my patch as your suggestion, please review it again.

Regards,
Leslie Zhai

[xiangzhai]

lib/Target/AVR/MCTargetDesc/AVRMCELFStreamer.h

42 ↗

(On Diff #120182)

I don't want to override EmitValue, sorry for my FunctionName misleading!

test/MC/AVR/relocations.s

81

Thanks :)

[dylanmckay]

@xiangzhai once the new comments are addressed, this should be good to commit

lib/Target/AVR/AsmParser/AVRAsmParser.cpp

609

Nitpick: Can we move this up to line 624 where we get the parser?

630

Same here regarding moving the variable up to 624

lib/Target/AVR/MCTargetDesc/AVRFixupKinds.h

119 ↗

(On Diff #124321)

We can probably remove this comment now that the fixups are always lowered to relocations

lib/Target/AVR/MCTargetDesc/AVRMCExpr.cpp

111 ↗

(On Diff #124321)

If I remember correctly, HH8 means bit 16-23, and so this mask will mask out the bits we actually want

Change the mask to 0xff0000

115 ↗

(On Diff #124321)

Same here, but mask should be 0xff000000

127 ↗

(On Diff #124321)

This needs the updated mask from the other HH8 fixup

[xiangzhai]

Dear Dylan,

Please review it again, thanks a lot!

Regards,
Leslie Zhai

[dylanmckay]

Patch is looking great, thanks Leslie!

lib/Target/AVR/AsmParser/AVRAsmParser.cpp

 //===---- AVRAsmParser.cpp - Parse AVR assembly to MCInst instructions ----===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "AVR.h"
 #include "AVRRegisterInfo.h"
 #include "MCTargetDesc/AVRMCExpr.h"
 #include "MCTargetDesc/AVRMCTargetDesc.h"
 
 #include "llvm/ADT/APInt.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCExpr.h"
 #include "llvm/MC/MCInst.h"
 #include "llvm/MC/MCInstBuilder.h"
 #include "llvm/MC/MCParser/MCAsmLexer.h"
 #include "llvm/MC/MCParser/MCParsedAsmOperand.h"
 #include "llvm/MC/MCParser/MCTargetAsmParser.h"
 #include "llvm/MC/MCStreamer.h"
 #include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/MC/MCSymbol.h"
 #include "llvm/MC/MCValue.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/TargetRegistry.h"
 
 #include <sstream>
 
 #define DEBUG_TYPE "avr-asm-parser"
 
 namespace llvm {
 
 /// Parses AVR assembly from a stream.
 class AVRAsmParser : public MCTargetAsmParser {
   const MCSubtargetInfo &STI;
   MCAsmParser &Parser;
   const MCRegisterInfo *MRI;
 
 #define GET_ASSEMBLER_HEADER
 #include "AVRGenAsmMatcher.inc"
 
   bool MatchAndEmitInstruction(SMLoc IDLoc, unsigned &Opcode,
                                OperandVector &Operands, MCStreamer &Out,
                                uint64_t &ErrorInfo,
                                bool MatchingInlineAsm) override;
 
   bool ParseRegister(unsigned &RegNo, SMLoc &StartLoc, SMLoc &EndLoc) override;
 
   bool ParseInstruction(ParseInstructionInfo &Info, StringRef Name,
                         SMLoc NameLoc, OperandVector &Operands) override;
 
   bool ParseDirective(AsmToken directiveID) override;
 
   OperandMatchResultTy parseMemriOperand(OperandVector &Operands);
 
   bool parseOperand(OperandVector &Operands);
   int parseRegisterName(unsigned (*matchFn)(StringRef));
   int parseRegisterName();
   int parseRegister();
   bool tryParseRegisterOperand(OperandVector &Operands);
   bool tryParseExpression(OperandVector &Operands);
   bool tryParseRelocExpression(OperandVector &Operands);
   void eatComma();
 
   unsigned validateTargetOperandClass(MCParsedAsmOperand &Op,
                                       unsigned Kind) override;
 
   unsigned toDREG(unsigned Reg, unsigned From = AVR::sub_lo) {
     MCRegisterClass const *Class = &AVRMCRegisterClasses[AVR::DREGSRegClassID];
     return MRI->getMatchingSuperReg(Reg, From, Class);
   }
 
   bool emit(MCInst &Instruction, SMLoc const &Loc, MCStreamer &Out) const;
   bool invalidOperand(SMLoc const &Loc, OperandVector const &Operands,
                       uint64_t const &ErrorInfo);
   bool missingFeature(SMLoc const &Loc, uint64_t const &ErrorInfo);
 
+  bool parseLiteralValues(unsigned Size, SMLoc L);
+
 public:
   AVRAsmParser(const MCSubtargetInfo &STI, MCAsmParser &Parser,
                const MCInstrInfo &MII, const MCTargetOptions &Options)
       : MCTargetAsmParser(Options, STI), STI(STI), Parser(Parser) {
     MCAsmParserExtension::Initialize(Parser);
     MRI = getContext().getRegisterInfo();
 
     setAvailableFeatures(ComputeAvailableFeatures(STI.getFeatureBits()));
if (Parser.getTok().getKind() != AsmToken::Identifier ||
     return true;
   }
   StringRef ModifierName = Parser.getTok().getString();
   ModifierKind = AVRMCExpr::getKindByName(ModifierName.str().c_str());
 
   if (ModifierKind != AVRMCExpr::VK_AVR_None) {
     Parser.Lex();
     Parser.Lex(); // Eat modifier name and parenthesis
+    // FIXME: Not support gs(foo) so just eat gs.
+    if (Parser.getTok().getString() == "gs" &&
+        Parser.getTok().getKind() == AsmToken::Identifier) {
+      Parser.Lex();
+    }
   } else {
     return Error(Parser.getTok().getLoc(), "unknown modifier");
   }
 
   MCExpr const *InnerExpression;
   if (getParser().parseExpression(InnerExpression))
     return true;
 
if (parseOperand(Operands)) {
       Parser.eatToEndOfStatement();
       return Error(Loc, "unexpected token in argument list");
     }
   }
   Parser.Lex(); // Consume the EndOfStatement
   return false;
 }
 
-bool AVRAsmParser::ParseDirective(llvm::AsmToken DirectiveID) { return true; }
+bool AVRAsmParser::ParseDirective(llvm::AsmToken DirectiveID) {
+  StringRef IDVal = DirectiveID.getIdentifier();
+  if (IDVal == ".word")
+    parseLiteralValues(4, DirectiveID.getLoc());
+  else if (IDVal == ".short")
+    parseLiteralValues(2, DirectiveID.getLoc());
+  else if (IDVal == ".byte")
+    parseLiteralValues(1, DirectiveID.getLoc());
+  return true;
+}
+
+bool AVRAsmParser::parseLiteralValues(unsigned Size, SMLoc L) {
+  MCAsmParser &Parser = getParser();
+  if (Parser.getTok().getKind() == AsmToken::Identifier &&
+      Parser.getLexer().peekTok().getKind() == AsmToken::LParen) {
+    const MCExpr *Value;
+    return Parser.parseExpression(Value);
+  }
+  auto parseOne = [&]() -> bool {
+    const MCExpr *Value;
+    if (Parser.parseExpression(Value))
+      return true;
+    Parser.getStreamer().EmitValue(Value, Size, L);
+    return false;
+  };
+  return (parseMany(parseOne));
+}
 
 extern "C" void LLVMInitializeAVRAsmParser() {
   RegisterMCAsmParser<AVRAsmParser> X(getTheAVRTarget());
 }
 
 #define GET_REGISTER_MATCHER
 #define GET_MATCHER_IMPLEMENTATION
 #include "AVRGenAsmMatcher.inc"
 
 // Uses enums defined in AVRGenAsmMatcher.inc
 unsigned AVRAsmParser::validateTargetOperandClass(MCParsedAsmOperand &AsmOp,
                                                   unsigned ExpectedKind) {
   AVROperand &Op = static_cast<AVROperand &>(AsmOp);
   MatchClassKind Expected = static_cast<MatchClassKind>(ExpectedKind);
 
   // If need be, GCC converts bare numbers to register names
   // It's ugly, but GCC supports it.
   if (Op.isImm()) {
     if (MCConstantExpr const *Const = dyn_cast<MCConstantExpr>(Op.getImm())) {
       int64_t RegNum = Const->getValue();
       std::ostringstream RegName;
       RegName << "r" << RegNum;

test/MC/AVR/relocations.s

 ; CHECK-NEXT: R_AVR_LO8_LDI_PM_NEG
 ldi r25, -pm_lo8(foo)
 
 ; CHECK-NEXT: R_AVR_HI8_LDI_PM_NEG
 ldi r25, -pm_hi8(foo)
 
 ; CHECK-NEXT: R_AVR_HH8_LDI_PM_NEG
 ldi r25, -pm_hh8(foo)
+
+; CHECK-NEXT: R_AVR_LO8_LDI_GS
+ldi r17, lo8(gs(foo))
+
+; CHECK-NEXT: R_AVR_HI8_LDI_GS
+ldi r18, hi8(gs(foo))
+
+; CHECK-NEXT: R_AVR_16_PM
+.short foo
+
+; CHECK-NEXT: R_AVR_8
+.byte foo
+
+; CHECK-NEXT: R_AVR_8_LO8
+.byte lo8(foo)
+
+; CHECK-NEXT: R_AVR_8_HI8
+.byte hi8(foo)
+
+; CHECK-NEXT: R_AVR_8_HLO8
+.byte hlo8(foo)