[PATCH] D125036: [RISCV] Alignment relaxation

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/Arch/RISCV.cpp:496
+
+unsigned getAlignBoundary(Relocation &r) {
+  return r.addend >> alignNopBytesWidth;
----------------
gkm wrote:
> MaskRay wrote:
> > static
> > 
> > Actually, avoid defining such a short function.
> What do you propose as an alternative? Open-code the shift?
Yes, open-code it.


================
Comment at: lld/ELF/Arch/RISCV.cpp:501
+void setAlignNopBytes(Relocation &r, unsigned n) {
+  r.addend = (r.addend & ~alignNopBytesMask) | n;
+}
----------------
gkm wrote:
> MaskRay wrote:
> > avoid defining such a short function.
> What do you propose as an alternative? Open-code the bitwise ops? The idea is to abstract access to subfields of `addend`. Ideally, I could access `alignNopBytes` and `alignBoundary` as distinct struct members, but I am hijacking the existing `Relocation` interface that I can't change.
Open code it since it's only called once. A comment is sufficient to express the intent.


================
Comment at: lld/ELF/InputSection.cpp:1100
     const Relocation &rel = relocations[i];
-    if (rel.expr == R_NONE)
+    if (rel.expr == R_NONE || rel.expr == R_RELAX_HINT)
       continue;
----------------
gkm wrote:
> MaskRay wrote:
> > Prefer moving R_RELAX_HINT to the switch below to not penalize other architectures. They don't want to take a comparison overhead here.
> I am unconvinced:
> # The extra comparison overhead will be lost in the noise, though I will measure to be certain
> # Delaying `R_RELAX_HINT` means we run the block of code between here and the `switch`, which requires handling `R_RELAX_HINT` in `InputSectionBase::getRelocTargetVA()` to return a fake value `0`.
> 
> 
R_NONE is also not nice here. It would be better be placed in the switch cases but I haven't got time to refine the basic-block-section usage.

R_RELAX_HINT is risc-v specific. I don't want it to penalize other code. The cost is certainly small but every a little makes a mickle.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125036/new/

https://reviews.llvm.org/D125036