[PATCH] D38029: [AVR] Override ParseDirective

[Dylan McKay via Phabricator via llvm-commits]

dylanmckay added a comment.

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!



================
Comment at: lib/Target/AVR/AsmParser/AVRAsmParser.cpp:14
 #include "MCTargetDesc/AVRMCTargetDesc.h"
+#include "MCTargetDesc/AVRMCELFStreamer.h"
 
----------------
These headers should be in alphabetical order


================
Comment at: lib/Target/AVR/AsmParser/AVRAsmParser.cpp:60
+  bool ParseDirective(AsmToken DirectiveID
+#ifdef REFACTORY_PARSEDIRECTIVE
+          , OperandVector &Operands
----------------
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




================
Comment at: lib/Target/AVR/MCTargetDesc/AVRAsmBackend.cpp:270
+  case AVR::fixup_lo8_ldi_gs:
+    if (Kind == AVR::fixup_lo8_ldi_pm || Kind == AVR::fixup_lo8_ldi_gs)
+      adjust::pm(Value);
----------------
`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.


================
Comment at: lib/Target/AVR/MCTargetDesc/AVRAsmBackend.cpp:277
   case AVR::fixup_hi8_ldi_pm:
-    if (Kind == AVR::fixup_hi8_ldi_pm) adjust::pm(Value);
+  case AVR::fixup_hi8_ldi_gs:
+    if (Kind == AVR::fixup_hi8_ldi_pm || Kind == AVR::fixup_hi8_ldi_gs)
----------------
Same here regarding `gs`


================
Comment at: lib/Target/AVR/MCTargetDesc/AVRELFObjectWriter.cpp:19
 #include "llvm/MC/MCValue.h"
+#include "llvm/MC/MCObjectWriter.h"
 #include "llvm/Support/ErrorHandling.h"
----------------
Place this in alphabetical order


================
Comment at: lib/Target/AVR/MCTargetDesc/AVRMCELFStreamer.h:1
+//===--------- AVRMCELFStreamer.h - AVR subclass of MCElfStreamer ---------===//
+//
----------------
`s/MCElfStreamer/MCELFStreamer`


================
Comment at: lib/Target/AVR/MCTargetDesc/AVRMCELFStreamer.h:13
+
+#include "MCTargetDesc/AVRMCTargetDesc.h"
+#include "MCTargetDesc/AVRMCExpr.h"
----------------
Header files should be in alphabetical order


================
Comment at: lib/Target/AVR/MCTargetDesc/AVRMCELFStreamer.h:39
+                   MCAssembler *Assembler) :
+  MCELFStreamer(Context, std::move(TAB), OS, std::move(Emitter)),
+  MCII(createAVRMCInstrInfo()) {}
----------------
Indent this constructor list like the one above


================
Comment at: lib/Target/AVR/MCTargetDesc/AVRMCELFStreamer.h:42
+
+  void EmitValue(const MCSymbol *Sym, unsigned SizeInBytes,
+                 SMLoc Loc = SMLoc(),
----------------
Pop an `override` qualifier on this function


================
Comment at: lib/Target/AVR/MCTargetDesc/AVRMCExpr.cpp:137
   }
-  return static_cast<uint64_t>(Value) & 0xff;
+  return static_cast<uint64_t>(Value) & 0xffff;
 }
----------------
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.



================
Comment at: lib/Target/AVR/MCTargetDesc/AVRMCTargetDesc.cpp:18
 #include "AVRTargetStreamer.h"
+#include "AVRMCELFStreamer.h"
 #include "InstPrinter/AVRInstPrinter.h"
----------------
Alphabetical order


================
Comment at: lib/Target/AVR/MCTargetDesc/AVRMCTargetDesc.cpp:26
 #include "llvm/MC/MCSubtargetInfo.h"
+#include "llvm/MC/MCAsmBackend.h"
+#include "llvm/MC/MCCodeEmitter.h"
----------------
Alphabetical order


================
Comment at: lib/Target/AVR/MCTargetDesc/CMakeLists.txt:10
   AVRTargetStreamer.cpp
+  AVRMCELFStreamer.cpp
 )
----------------
Add this in alphabetical order


================
Comment at: test/MC/AVR/relocations.s:108
+
+; CHECK-NEXT: R_AVR_HI8_LDI_GS
+ldi r18, hi8(gs(foo))
----------------
Nice test


Repository:
  rL LLVM

https://reviews.llvm.org/D38029