[PATCH] D23568: [RISCV 10/10] Add common fixups and relocations

[Dylan McKay via Phabricator via llvm-commits]

dylanmckay added inline comments.


================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:169
+      return isInt<12>(getConstantImm());
+    } else if (isImm()) {
+      RISCVMCExpr::VariantKind VK;
----------------
No `else` after return


================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:183
 
   bool isSImm13Lsb0() const {
+    if (isConstantImm()) {
----------------
Can we factor any code out from both `isSImm13Lsb0` and the other signed immediated lsb functions? It seems like the only difference in these functions are the sizes of the immediates

If the same could be done for the UImm functions, that'd be good, although a quick glance shows that there is some differing logic based on the variant


================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:186
+      return isShiftedInt<12, 1>(getConstantImm());
+    } else if (isImm()) {
+      RISCVMCExpr::VariantKind VK;
----------------
No else after return


================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:197
+      return isUInt<20>(getConstantImm());
+    } else if (isImm()) {
+      RISCVMCExpr::VariantKind VK;
----------------
No else after return


================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:210
+      return isShiftedInt<20, 1>(getConstantImm());
+    } else if (isImm()) {
+      RISCVMCExpr::VariantKind VK;
----------------
No else after return


================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:273
     auto Op = make_unique<RISCVOperand>(Immediate);
+    if (const RISCVMCExpr *RE = dyn_cast<RISCVMCExpr>(Val)) {
+      int64_t Res;
----------------
Is there a reason why this is only executed for `RISCVMCExpr` objects? AFAIK, `evaluateAsConstant` is also supported by `MCExpr`, and so I'm not sure if you're intentionally not evaluating those expressions here.


================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:477
+  }
+  StringRef Identifier = getParser().getTok().getString();
+  RISCVMCExpr::VariantKind VK = RISCVMCExpr::getVariantKindForName(Identifier);
----------------
Should this be `getIdentifier()`?

It seems strange that you'd verify that the token is an identifier in the above `if`, but then read a string instead. 


================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp:36
 STATISTIC(MCNumEmitted, "Number of MC instructions emitted");
+STATISTIC(MCNumFixups, "Number of MC fixups created.");
 
----------------
I don't think we want a full stop here because it might muck up the output of `llc --stats`, or at least make it look funny


https://reviews.llvm.org/D23568