[PATCH] D125036: [RISCV] Alignment relaxation

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/Arch/RISCV.cpp:40
                 uint64_t val) const override;
+  void finalizeSections() const override;
 };
----------------
gkm wrote:
> MaskRay wrote:
> > The name is unnecessarily the same as `Writer::finalizeSections`. `relaxSections` may be a better name.
> This is an override of target-independent `virtual void TargetInfo::finalizeSections()`, which is meant to be a generic hook for targets to do target-specific things. For RISC-V, that target-specific thing happens to be relaxation.
My point is that I don't think other ports will do similar things. Rename this to `relaxSections` to make debugging easier. We can rename to `finalizeSections` if there is ever a need.


================
Comment at: lld/test/ELF/riscv-relax-align.s:39
+.balign 4
+  add t0, t1, t2
----------------
MaskRay wrote:
> Add a few more text sections to demonstrate that multiple sections can be handled.
Not done? As mentioned, this test only has 12-byte nops in one place. This is not strong enough to catch issues.
We need multiple `.blign` in this section and `.blign` in other text sections.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125036