[PATCH] D125036: [RISCV] Alignment relaxation

[Greg McGary via Phabricator via llvm-commits]

gkm marked 17 inline comments as done.
gkm added a comment.

In D125036#3513877 <https://reviews.llvm.org/D125036#3513877>, @MaskRay wrote:

> Clone https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git , apply this patch:

. . .

> I see a segfault

Yes, I see it also. Will debug.
Meanwhile, I responded to other feedback and asked you some follow-up questions.
Thanks for reviewing!



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


================
Comment at: lld/ELF/Arch/RISCV.cpp:501
+void setAlignNopBytes(Relocation &r, unsigned n) {
+  r.addend = (r.addend & ~alignNopBytesMask) | n;
+}
----------------
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.


================
Comment at: lld/ELF/InputSection.h:154
 
+  mutable bool copiedData = false;
+  mutable ArrayRef<uint8_t> rawData;
----------------
MaskRay wrote:
> reames wrote:
> > Having variables defined between functions looks odd to my eye, but I see this already exists in the class.  Maybe do a pre-nfc to group all the data together, and then add your new ones there?
> The mutable can be avoided.
Say more. How?


================
Comment at: lld/test/ELF/riscv-relax-align-rvc.s:11
+# RUN: ld.lld %t/rv64.o -o %t/relax.rv64
+# RUN: llvm-objdump -d -M no-aliases %t/relax.rv32 > %t/relax.rv32.dis
+# RUN: llvm-objdump -d -M no-aliases %t/relax.rv64 > %t/relax.rv64.dis
----------------
MaskRay wrote:
> Avoid a temporary file for FIleCheck input.
I find that the temporary files are very useful for debugging tests. I can see the output of `llvm-objdump` without the effort of extracting the command from the log and running it manually.

Although I wish it were standard practice to retain temp files, I can revert back to a pipeline prior to pushing upstream.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125036