[PATCH] D38029: [AVR] Override ParseDirective

[Dylan McKay via Phabricator via llvm-commits]

dylanmckay added a comment.

This patch is looking really nice by the way.



================
Comment at: lib/Target/AVR/AsmParser/AVRAsmParser.cpp:441
     Parser.Lex(); // Eat modifier name and parenthesis
+    if (Parser.getTok().getString() == "gs" &&
+        Parser.getTok().getKind() == AsmToken::Identifier) {
----------------
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


================
Comment at: lib/Target/AVR/AsmParser/AVRAsmParser.cpp:612
+  if (IDVal.lower() == ".long")
+    parseLiteralValues(8, DirectiveID.getLoc()/*, Operands*/);
+  else if (IDVal.lower() == ".word")
----------------
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](https://gcc.gnu.org/wiki/avr-gcc#Type_Layout) is a document detailing the AVR integer types and their sizes


================
Comment at: lib/Target/AVR/AsmParser/AVRAsmParser.cpp:622
+
+bool AVRAsmParser::parseLiteralValues(unsigned Size, SMLoc L/*, OperandVector &Operands*/) {
+  MCAsmParser &Parser = getParser();
----------------
Can we be more specific and name `Size` -> `SizeInBytes`


================
Comment at: lib/Target/AVR/AsmParser/AVRAsmParser.cpp:642
+    SMLoc E = SMLoc::getFromPointer(Parser.getTok().getLoc().getPointer() - 1);
+    // FIXME: not supports .byte lo8(foo)
+    //Operands.push_back(AVROperand::CreateImm(Expression, S, E));
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D38029