[PATCH] D125036: [RISCV] Alignment relaxation

[Greg McGary via Phabricator via llvm-commits]

gkm marked an inline comment as done.
gkm added inline comments.


================
Comment at: lld/ELF/Arch/RISCV.cpp:571-573
+// Execute a single relaxation pass. Return TRUE if we did something.  The
+// caller will repeatedly call this until the algorithm converges, as indicated
+// by a pass where nothing happens.
----------------
luismarques wrote:
> What expectations should we have about the convergence? Let's write some of that down. Either in this comment or in some place that provides a more global overview.
I will elaborate on that in later diffs. Alignment alone finishes in one pass.


================
Comment at: lld/ELF/Arch/RISCV.cpp:539-540
+//
+// Fill the gaps created by adding bytes (when delta > 0) to the section:
+// * After a relaxation was undone, restore the original instruction.
+// * After the alignment gap length is known, fill with NOPs.
----------------
luismarques wrote:
> Sorry, where is this done?
I should modify the comment because it pertains to non-alignment relaxation types that are implemented in subsequent diffs.


================
Comment at: lld/ELF/InputSection.cpp:178
+        if (auto *isec = dyn_cast_or_null<InputSection>(d->section))
+          if (isec->flags & SHF_EXECINSTR) {
+            SymbolAddrs &symbolAddrs = sectionSymbolAddrs[isec];
----------------
luismarques wrote:
> This was changed but the tests didn't change. Sounds like an opportunity to improve test coverage?
The change is an optimization, and does not alter behavior, so no new test is necessary. Before, I was doing unnecessary work to store symbol boundaries for non-executable sections which would never be relaxed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125036