[PATCH] D42515: [RISCV] Add support for %pcrel_lo.

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

Hi Ahmed. Many thanks for the contribution, and sorry for the slight delay in reviewing.

There are a couple of things you can do to make it easier to review:

- Follow the advice on requesting a review via phabricator <https://www.llvm.org/docs/Phabricator.html#phabricator-request-review-web> and generate a patch with full context. This makes it much easier to review from the web interface
- Make use of clang-format to ensure indentation etc matches the LLVM coding style. See here <https://www.llvm.org/docs/Contributing.html>. Though note clang-format will try to reformat the table in RISCVAsmBackend.cpp - revert that change. It might actually be worth adding comments to disable clang-format <https://clang.llvm.org/docs/ClangFormatStyleOptions.html#disabling-formatting-on-a-piece-of-code> around that table.

Some minor formatting issues, but otherwise this is looking good to me once those are addressed. Thanks!



================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVELFObjectWriter.cpp:58-61
+  case RISCV::fixup_riscv_pcrel_lo12_i:
+      return ELF::R_RISCV_PCREL_LO12_I;
+  case RISCV::fixup_riscv_pcrel_lo12_s:
+      return ELF::R_RISCV_PCREL_LO12_S;
----------------
Indentation is incorrect (clang-format is your friend!)


================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp:170-174
+    case RISCVMCExpr::VK_RISCV_PCREL_LO:
+        FixupKind = MIFrm == RISCVII::InstFormatI ? RISCV::fixup_riscv_pcrel_lo12_i
+                                                  : RISCV::fixup_riscv_pcrel_lo12_s;
+        break;
+        break;
----------------
Duplicated break, and clang-format will suggest the slightly nicer:


```

+      FixupKind = MIFrm == RISCVII::InstFormatI
+                      ? RISCV::fixup_riscv_pcrel_lo12_i
+                      : RISCV::fixup_riscv_pcrel_lo12_s;

```


================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVMCExpr.cpp:70-72
+      return "pcrel_lo";
   case VK_RISCV_PCREL_HI:
+      return "pcrel_hi";
----------------
Over-indented


Repository:
  rL LLVM

https://reviews.llvm.org/D42515