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

[Simon Cook via Phabricator via llvm-commits]

simoncook added inline comments.


================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp:42
+  bool requiresDiffExpressionRelocations() const override {
+    return STI.getFeatureBits()[RISCV::FeatureRelax];
+  }
----------------
shiva0217 wrote:
> 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?
I'm not quite sure what you mean. Compiling this test case I see ADD16 and SUB16 relocations in the table when relaxation is enabled, which I would expect. I did notice clang crashes assembling this case with relaxation disabled, I've submitted a fix to this as D46746, was it this crash you were referring to/the fixup being applied there is fine?


Repository:
  rL LLVM

https://reviews.llvm.org/D45181