[LoongArch] Use R_LARCH_ALIGN without symbol as much as possible #93775

SixWeining · 2024-05-30T06:59:02Z

To support the third parameter of the alignment directive, R_LARCH_ALIGN relocations need a non-zero symbol index.
In many cases we don't need the third parameter and can set the symbol index to 0.
This patch will remove a lot of .Lla-relax-align* symbols and mitigate the size regression due to #72962.

This patch is mainly from He Jinyang hejinyang@loongson.cn, also of Loongson.

Co-authored-by: Weining Lu luweining@loongson.cn

In LoongArch psABI v2.30, the R_LARCH_ALIGN requires symbol index to to support the third parameter of alignment directive. Following this extended R_LARCH_ALIGN make the symbol index be redundant in some case. E.g. `.align 4` or `.align 4,,16`. Thus, use the extended R_LARCH_ALIGN only when the third parameter is effective. Otherwise use the orginal R_LARCH_ALIGN for simple.

llvmbot · 2024-05-30T06:59:31Z

@llvm/pr-subscribers-backend-loongarch
@llvm/pr-subscribers-mc
@llvm/pr-subscribers-lld

@llvm/pr-subscribers-lld-elf

Author: Lu Weining (SixWeining)

Changes

In LoongArch psABI v2.30, the R_LARCH_ALIGN requires symbol index to to support the third parameter of alignment directive. Following this extended R_LARCH_ALIGN make the symbol index be redundant in some case. E.g. .align 4 or .align 4,,16. Thus, use the extended R_LARCH_ALIGN only when the third parameter is effective. Otherwise use the orginal R_LARCH_ALIGN for simple.

This patch is from He Jinyang <hejinyang@loongson.cn>, also of Loongson.

Full diff: https://github.com/llvm/llvm-project/pull/93775.diff

4 Files Affected:

(modified) lld/test/ELF/loongarch-relax-emit-relocs.s (+2-2)
(modified) llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchAsmBackend.cpp (+17-12)
(modified) llvm/test/MC/LoongArch/Relocations/relax-addsub.s (+1-1)
(modified) llvm/test/MC/LoongArch/Relocations/relax-align.s (+7-5)

diff --git a/lld/test/ELF/loongarch-relax-emit-relocs.s b/lld/test/ELF/loongarch-relax-emit-relocs.s
index 581fce8c95caa..ba414e8c93f0f 100644
--- a/lld/test/ELF/loongarch-relax-emit-relocs.s
+++ b/lld/test/ELF/loongarch-relax-emit-relocs.s
@@ -25,7 +25,7 @@
 # CHECK-NEXT:     R_LARCH_PCALA_LO12 _start
 # CHECK-NEXT:     R_LARCH_RELAX *ABS*
 # CHECK-NEXT:   nop
-# CHECK-NEXT:     R_LARCH_ALIGN .Lla-relax-align0+0x4
+# CHECK-NEXT:     R_LARCH_ALIGN *ABS*+0xc
 # CHECK-NEXT:   nop
 # CHECK-NEXT:   ret
 
@@ -37,7 +37,7 @@
 # CHECKR-NEXT:     R_LARCH_PCALA_LO12 _start
 # CHECKR-NEXT:     R_LARCH_RELAX *ABS*
 # CHECKR-NEXT:   nop
-# CHECKR-NEXT:     R_LARCH_ALIGN .Lla-relax-align0+0x4
+# CHECKR-NEXT:     R_LARCH_ALIGN *ABS*+0xc
 # CHECKR-NEXT:   nop
 # CHECKR-NEXT:   nop
 # CHECKR-NEXT:   ret
diff --git a/llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchAsmBackend.cpp b/llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchAsmBackend.cpp
index de492f2b1f0a4..1006cc42ad2ef 100644
--- a/llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchAsmBackend.cpp
+++ b/llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchAsmBackend.cpp
@@ -224,20 +224,25 @@ bool LoongArchAsmBackend::shouldInsertFixupForCodeAlign(
   // Create fixup_loongarch_align fixup.
   MCFixup Fixup =
       MCFixup::create(0, Dummy, MCFixupKind(LoongArch::fixup_loongarch_align));
-  const MCSymbolRefExpr *MCSym = getSecToAlignSym()[Sec];
-  if (MCSym == nullptr) {
-    // Create a symbol and make the value of symbol is zero.
-    MCSymbol *Sym = Ctx.createNamedTempSymbol("la-relax-align");
-    Sym->setFragment(&*Sec->getBeginSymbol()->getFragment());
-    Asm.registerSymbol(*Sym);
-    MCSym = MCSymbolRefExpr::create(Sym, Ctx);
-    getSecToAlignSym()[Sec] = MCSym;
-  }
+
+  auto createExtendedValue = [&]() {
+    const MCSymbolRefExpr *MCSym = getSecToAlignSym()[Sec];
+    if (MCSym == nullptr) {
+      // Create a symbol and make the value of symbol is zero.
+      MCSymbol *Sym = Ctx.createNamedTempSymbol("la-relax-align");
+      Sym->setFragment(&*Sec->getBeginSymbol()->getFragment());
+      Asm.registerSymbol(*Sym);
+      MCSym = MCSymbolRefExpr::create(Sym, Ctx);
+      getSecToAlignSym()[Sec] = MCSym;
+    }
+    unsigned Lo = Log2_64(Count) + 1;
+    unsigned Hi = AF.getMaxBytesToEmit();
+    return MCValue::get(MCSym, nullptr, Hi << 8 | Lo);
+  };
 
   uint64_t FixedValue = 0;
-  unsigned Lo = Log2_64(Count) + 1;
-  unsigned Hi = AF.getMaxBytesToEmit() >= Count ? 0 : AF.getMaxBytesToEmit();
-  MCValue Value = MCValue::get(MCSym, nullptr, Hi << 8 | Lo);
+  MCValue Value = AF.getMaxBytesToEmit() >= Count ? MCValue::get(Count)
+                                                  : createExtendedValue();
   Asm.getWriter().recordRelocation(Asm, Layout, &AF, Fixup, Value, FixedValue);
 
   return true;
diff --git a/llvm/test/MC/LoongArch/Relocations/relax-addsub.s b/llvm/test/MC/LoongArch/Relocations/relax-addsub.s
index 18e0ede5e2937..f2524b29d230b 100644
--- a/llvm/test/MC/LoongArch/Relocations/relax-addsub.s
+++ b/llvm/test/MC/LoongArch/Relocations/relax-addsub.s
@@ -28,7 +28,7 @@
 
 # RELAX:       Relocations [
 # RELAX-NEXT:    Section ({{.*}}) .rela.text {
-# RELAX-NEXT:      0x4 R_LARCH_ALIGN {{.*}} 0x4
+# RELAX-NEXT:      0x4 R_LARCH_ALIGN - 0xC
 # RELAX-NEXT:      0x10 R_LARCH_PCALA_HI20 .L1 0x0
 # RELAX-NEXT:      0x10 R_LARCH_RELAX - 0x0
 # RELAX-NEXT:      0x14 R_LARCH_PCALA_LO12 .L1 0x0
diff --git a/llvm/test/MC/LoongArch/Relocations/relax-align.s b/llvm/test/MC/LoongArch/Relocations/relax-align.s
index 294fd9fb916c7..477d5ca24ec7d 100644
--- a/llvm/test/MC/LoongArch/Relocations/relax-align.s
+++ b/llvm/test/MC/LoongArch/Relocations/relax-align.s
@@ -63,17 +63,19 @@ ret
 ## Test the symbol index is different from .text.
 .section .text2, "ax"
 .p2align 4
+.p2align 4, , 4
 break 7
 
 # RELOC:            Relocations [
 # RELAX-RELOC-NEXT:   Section ({{.*}}) .rela.text {
-# RELAX-RELOC-NEXT:     0x24 R_LARCH_ALIGN .Lla-relax-align0 0x4
-# RELAX-RELOC-NEXT:     0x34 R_LARCH_ALIGN .Lla-relax-align0 0x5
-# RELAX-RELOC-NEXT:     0x50 R_LARCH_ALIGN .Lla-relax-align0 0x4
+# RELAX-RELOC-NEXT:     0x24 R_LARCH_ALIGN - 0xC
+# RELAX-RELOC-NEXT:     0x34 R_LARCH_ALIGN - 0x1C
+# RELAX-RELOC-NEXT:     0x50 R_LARCH_ALIGN - 0xC
 # RELAX-RELOC-NEXT:     0x60 R_LARCH_ALIGN .Lla-relax-align0 0xB04
-# RELAX-RELOC-NEXT:     0x70 R_LARCH_ALIGN .Lla-relax-align0 0x4
+# RELAX-RELOC-NEXT:     0x70 R_LARCH_ALIGN - 0xC
 # RELAX-RELOC-NEXT:   }
 # RELAX-RELOC-NEXT:   Section ({{.*}}) .rela.text2 {
-# RELAX-RELOC-NEXT:     0x0 R_LARCH_ALIGN .Lla-relax-align1 0x4
+# RELAX-RELOC-NEXT:     0x0 R_LARCH_ALIGN - 0xC
+# RELAX-RELOC-NEXT:     0xC R_LARCH_ALIGN .Lla-relax-align1 0x404
 # RELAX-RELOC-NEXT:   }
 # RELOC-NEXT:       ]

llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchAsmBackend.cpp

MaskRay · 2024-05-30T23:17:43Z

llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchAsmBackend.cpp

+      MCSym = MCSymbolRefExpr::create(Sym, Ctx);
+      getSecToAlignSym()[Sec] = MCSym;
+    }
+    unsigned Lo = Log2_64(Count) + 1;


prefer countr_zero from llvm/ADT/bit.h

Thanks, but I find Log2(AF.getAlignment()) is simpler.

llvm/Support/Alignment.h:

/// Returns the log2 of the alignment. inline unsigned Log2(Align A) { return A.ShiftValue; }

MaskRay · 2024-05-30T23:21:54Z

"Squash and merge" assigns the author field to the PR author (not the patch author) and adds Co-authored-by tags for commit authors.
If you are a coauthor, you can set the Co-authored-by tag in the description.

In LoongArch psABI v2.30, the R_LARCH_ALIGN requires symbol index to to support the third parameter of alignment directive. Following this extended R_LARCH_ALIGN make the symbol index be redundant in some case. E.g. .align 4 or .align 4,,16. Thus, use the extended R_LARCH_ALIGN only when the third parameter is effective. Otherwise use the orginal R_LARCH_ALIGN for simple.

"... to support the third parameter of the alignment directive, R_LARCH_ALIGN relocations need a non-zero symbol index.
In many cases we don't need the third parameter and can set the symbol index to 0.
This patch will remove a lot of .Lla-relax-align* symbols and mitigate the size regression due to #72962."

llvmbot added lld mc Machine (object) code lld:ELF backend:loongarch labels May 30, 2024

SixWeining requested a review from MaskRay May 30, 2024 06:59

MaskRay reviewed May 30, 2024

View reviewed changes

llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchAsmBackend.cpp Outdated Show resolved Hide resolved

MaskRay reviewed May 30, 2024

View reviewed changes

MaskRay approved these changes May 30, 2024

View reviewed changes

Address @MaskRay's comments

3c3c763

SixWeining merged commit fbdd948 into llvm:main Jun 4, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[LoongArch] Use R_LARCH_ALIGN without symbol as much as possible #93775

[LoongArch] Use R_LARCH_ALIGN without symbol as much as possible #93775

SixWeining commented May 30, 2024 •

edited

Loading

llvmbot commented May 30, 2024 •

edited

Loading

MaskRay May 30, 2024

SixWeining May 31, 2024

MaskRay commented May 30, 2024

[LoongArch] Use R_LARCH_ALIGN without symbol as much as possible #93775

[LoongArch] Use R_LARCH_ALIGN without symbol as much as possible #93775

Conversation

SixWeining commented May 30, 2024 • edited Loading

llvmbot commented May 30, 2024 • edited Loading

MaskRay May 30, 2024

Choose a reason for hiding this comment

SixWeining May 31, 2024

Choose a reason for hiding this comment

MaskRay commented May 30, 2024

SixWeining commented May 30, 2024 •

edited

Loading

llvmbot commented May 30, 2024 •

edited

Loading