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

[Shiva Chen via Phabricator via llvm-commits]

shiva0217 added inline comments.


================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp:42
+  bool requiresDiffExpressionRelocations() const override {
+    return STI.getFeatureBits()[RISCV::FeatureRelax];
+  }
----------------
asb wrote:
> simoncook wrote:
> > I've been doing some additional thinking on this, and I don't know if having this depend on the relax feature bit might introduce a problem going into the future.
> > 
> > Looking at D45864, the way `.option rvc` and `.option norvc` are implemented is they add and remove feature bits. If in the future we support `.option norelax` through the same approach we can end up with a situation where we end up with correct DWARF depending on which of these two is used last.
> > 
> > If we want to do this unconditionally, then we need to track whether feature relax was ever enabled, possibly via a second feature bit that is set by `.option relax` but not cleared by `.option norelax` and have return true if either is set. Does this seem like a good approach?
> I could see that argument. I could certainly see a counter-argument that says that if the user uses .option relax and .option norelax in the same section then they're doing something wrong and if things break they get to keep both pieces.
> 
> Maybe an alternative is to warn if relax/norelax is switched within the same section?
The array value in the following case will always generate a fixup instead of absolute value after the commit.

commit 4d344626681a31cd8993f97eceb68419bfcc4c54
Author:     Nirav Dave <niravd at google.com>
AuthorDate: Mon Apr 30 19:22:40 2018 +0000
Commit:     Nirav Dave <niravd at google.com>
CommitDate: Mon Apr 30 19:22:40 2018 +0000
[MC] Change AsmParser to leverage Assembler during evaluation
  int foo (int a)
  {
    static const short ar[] = { &&l1 - &&l1, &&l2 - &&l1 };
    void *p = &&l1 + ar[a];
    goto *p;
   l1:
    return 1;
   l2:
    return 2;
  }
We probably have two options.
1. requiresDiffExpressionRelocations() always return true to emit ADD16 and SUB16 relocations.
2. Fix up the above commit and then create a new patch to force this case to emit a relocation type when requiresDiffExpressionRelocations return true.

Any thoughts?


Repository:
  rL LLVM

https://reviews.llvm.org/D45181