[PATCH] D23771: [AAP] (5) Add AAP MC layer support

[Simon Cook via llvm-commits]

simoncook requested changes to this revision.
simoncook added a reviewer: simoncook.
simoncook added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Target/AAP/MCTargetDesc/AAPAsmBackend.cpp:32
+      // resolving them, these are more complex fields which we instead want
+      // populate in the linker.
+
----------------
instead want //to// populate?


================
Comment at: lib/Target/AAP/MCTargetDesc/AAPAsmBackend.cpp:89
+    for (unsigned i = 0; i < Size; ++i) {
+      Data[i + Fixup.getOffset()] = static_cast<uint8_t>(Value >> (i * 8));
+    }
----------------
This should be an |= in case you want to add support for any non-default fixup.


================
Comment at: lib/Target/AAP/MCTargetDesc/AAPAsmBackend.cpp:104
+    case FK_Data_8:
+      isResolved = true;
+    default:
----------------
This seems suspicious, so any fixed 1/2/4/8 bit fixup is guaranteed to be known?


================
Comment at: lib/Target/AAP/MCTargetDesc/AAPELFObjectWriter.cpp:62
+
+  case FK_Data_1: return ELF::R_AAP_8;
+  case FK_Data_2: return ELF::R_AAP_16;
----------------
Should these be aligned with the previous groups?


================
Comment at: lib/Target/AAP/MCTargetDesc/AAPMCCodeEmitter.cpp:75
+
+  assert(Kind == MCExpr::SymbolRef);
+
----------------
This could fail if you have an expression that either isn't binary (e.g. ~foo), or where the Symbol is on the RHS (e.g. 2+foo).

For example, 
```
bal ~foo, $r0
```

In this case, I'd expect generic emission code to tell me it expected something relocatable later on.



================
Comment at: lib/Target/AAP/MCTargetDesc/AAPMCTargetDesc.cpp:1
+//===-- AAPMCTargetDesc.cpp - AAP Target Descriptions ---------------===//
+//
----------------
nitpic: This line is short


================
Comment at: lib/Target/AAP/MCTargetDesc/AAPMCTargetDesc.h:1
+//===-- AAPMCTargetDesc.h - AAP Target Descriptions -------*- C++ -*-===//
+//
----------------
nitpic: This line is short


https://reviews.llvm.org/D23771