[PATCH] D45181: [RISCV] Add diff relocation support for RISC-V

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

Thanks Simon, I think this is in pretty good shape. I've added a few comments on code style and expanding the tests. The only part of the patch I'm unsure about is the reference to InSet, so if you can clarify I'd really appreciate it.



================
Comment at: include/llvm/MC/MCFixup.h:101
 
+  /// \brief Return a fixup corresponding to the add half of a add/sub fixup
+  /// pair for the given Fixup.
----------------
autobrief is enabled in the LLVM Doxygen config so `\brief` isn't needed in any of these newly added functions.


================
Comment at: include/llvm/MC/MCFixup.h:142-165
+  /// \brief Return the generic fixup kind for an addition with a given size. It
+  /// is an error to pass an unsupported size.
+  static MCFixupKind getAddKindForKind(unsigned Kind) {
+    switch (Kind) {
+      default: llvm_unreachable("Unknown type to convert!");
+      case FK_Data_1: return FK_Data_Add_1;
+      case FK_Data_2: return FK_Data_Add_2;
----------------
LLVM coding style is not to indent case labels (see getKindForSize).


================
Comment at: lib/MC/MCAssembler.cpp:699-701
     // The fixup was unresolved, we need a relocation. Inform the object
     // writer of the relocation, and give it an opportunity to adjust the
     // fixup value if need be.
----------------
This comment applies to both the if and else branches it should be brought up to just below   `if (!IsResolved) {` and possibly given a minor rephrasing to mention the requiresDiffExpressionRelocation case.


================
Comment at: lib/MC/MCAssembler.cpp:702
     // fixup value if need be.
-    getWriter().recordRelocation(*this, Layout, &F, Fixup, Target, FixedValue);
+    else
+      getWriter().recordRelocation(*this, Layout, &F, Fixup, Target,
----------------
Given that the if() has a brace, the else should probably have one too. [Note: personal preference - can't see guidance one way or another in the LLVM coding guidelines).


================
Comment at: lib/MC/MCExpr.cpp:574
+  if (Asm &&
+      (InSet || !Asm->getBackend().requiresDiffExpressionRelocations())) {
     // First, fold out any differences which are fully resolved. By
----------------
Why is InSet needed in this if statement? That's a genuine question rather than a suggestion it's wrong - the meaning of InSet is unclear.


================
Comment at: test/MC/RISCV/fixups-expr.s:5
+# RUN:     | llvm-readobj -r | FileCheck -check-prefix=NORELAX %s
+
+# Check that subtraction expressions are emitted as two relocations
----------------
Could you please make this test more exhaustive:

1) Add riscv64 check lines
2) Check R_RISCV_{ADD,SUB}{8,16,32} as well


================
Comment at: test/MC/RISCV/hilo-constaddr-expr.s:1
+# RUN: not llvm-mc -filetype=obj -triple=riscv32 -mattr=+relax %s 2>&1 | FileCheck %s
+
----------------
Could you add a RUN line for when relax is disabled, and adapt the CHECK lines that were previously there when this was in hilo-constaddr.s for this example?




Repository:
  rL LLVM

https://reviews.llvm.org/D45181