Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MipsAsmParser/O32: Don't add redundant $ to $-prefixed symbol in the la macro #80644

Merged
merged 4 commits into from
Feb 14, 2024

Conversation

wzssyqa
Copy link
Contributor

@wzssyqa wzssyqa commented Feb 5, 2024

When parsing the la macro, we add a duplicate $ prefix in getOrCreateSymbol,
leading to error: Undefined temporary symbol $$yy for code like:

xx:
	la	$2,$yy
$yy:
	nop

Remove the duplicate prefix.

In addition, recognize .L-prefixed symbols as local for O32.

See: #65020.

@llvmbot llvmbot added the mc Machine (object) code label Feb 5, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Feb 5, 2024

@llvm/pr-subscribers-mc

Author: YunQiang Su (wzssyqa)

Changes

For asm code like:
xx:
la $2,$yy
$yy:
nop

MIPS O32 use $ as PrivateGlobalPrefix, while another $ is added for getOrCreateSymbol, thus:
error: Undefined temporary symbol $$yy

We also set symbols that starts with .L as local for O32.

See: #65020.


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

2 Files Affected:

  • (modified) llvm/lib/Target/Mips/AsmParser/MipsAsmParser.cpp (+6-1)
  • (added) llvm/test/MC/Mips/macro-la-local.s (+23)
diff --git a/llvm/lib/Target/Mips/AsmParser/MipsAsmParser.cpp b/llvm/lib/Target/Mips/AsmParser/MipsAsmParser.cpp
index 3c673ae938fde..2ddd8a6f31756 100644
--- a/llvm/lib/Target/Mips/AsmParser/MipsAsmParser.cpp
+++ b/llvm/lib/Target/Mips/AsmParser/MipsAsmParser.cpp
@@ -2920,6 +2920,11 @@ bool MipsAsmParser::loadAndAddSymbolAddress(const MCExpr *SymExpr,
         (Res.getSymA()->getSymbol().isELF() &&
          cast<MCSymbolELF>(Res.getSymA()->getSymbol()).getBinding() ==
              ELF::STB_LOCAL);
+    if (!IsLocalSym && ABI.IsO32()) {
+      // PrivateGlobalPrefix for O32 is '$', while we support '.L' anyway.
+      if (Res.getSymA()->getSymbol().getName().starts_with(".L"))
+        IsLocalSym = true;
+    }
     bool UseXGOT = STI->hasFeature(Mips::FeatureXGOT) && !IsLocalSym;
 
     // The case where the result register is $25 is somewhat special. If the
@@ -6359,7 +6364,7 @@ bool MipsAsmParser::parseOperand(OperandVector &Operands, StringRef Mnemonic) {
       return true;
 
     SMLoc E = SMLoc::getFromPointer(Parser.getTok().getLoc().getPointer() - 1);
-    MCSymbol *Sym = getContext().getOrCreateSymbol("$" + Identifier);
+    MCSymbol *Sym = getContext().getOrCreateSymbol(Identifier);
     // Otherwise create a symbol reference.
     const MCExpr *SymRef =
         MCSymbolRefExpr::create(Sym, MCSymbolRefExpr::VK_None, getContext());
diff --git a/llvm/test/MC/Mips/macro-la-local.s b/llvm/test/MC/Mips/macro-la-local.s
new file mode 100644
index 0000000000000..14c8e1b689ac9
--- /dev/null
+++ b/llvm/test/MC/Mips/macro-la-local.s
@@ -0,0 +1,23 @@
+# RUN: llvm-mc %s -triple=mips-unknown-linux -show-encoding -mcpu=mips32r2 | \
+# RUN:   FileCheck %s --check-prefixes=CHECK
+# RUN: llvm-mc %s -triple=mips-unknown-linux -show-encoding -mcpu=mips32r6 | \
+# RUN:   FileCheck %s --check-prefixes=CHECK
+
+	.text
+	.abicalls
+	.option	pic2
+xx:
+	la	$2,.Lhello #CHECK: lw      $2, %got(.Lhello)($gp)          # encoding: [0x8f,0x82,A,A]
+                           #CHECK: #   fixup A - offset: 0, value: %got(.Lhello), kind: fixup_Mips_GOT
+                           #CHECK: addiu   $2, $2, %lo(.Lhello)            # encoding: [0x24,0x42,A,A]
+                           #CHECK: #   fixup A - offset: 0, value: %lo(.Lhello), kind: fixup_Mips_LO16
+
+	la	$2,$hello2 #CHECK: lw      $2, %got($hello2)($gp)          # encoding: [0x8f,0x82,A,A]
+                           #CHECK: #   fixup A - offset: 0, value: %got($hello2), kind: fixup_Mips_GOT
+                           #CHECK: addiu   $2, $2, %lo($hello2)            # encoding: [0x24,0x42,A,A]
+                           #CHECK: #   fixup A - offset: 0, value: %lo($hello2), kind: fixup_Mips_LO16
+	.rdata
+.Lhello:
+	.asciz "Hello world\n"
+$hello2:
+	.asciz "Hello world\n"

@wzssyqa
Copy link
Contributor Author

wzssyqa commented Feb 6, 2024

@MaskRay @brad0

.abicalls
.option pic2
xx:
la $2,.Lhello #CHECK: lw $2, %got(.Lhello)($gp) # encoding: [0x8f,0x82,A,A]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

# CHECK:      lw ...
# CHECK-NEXT:   fixup A ...
# CHECK-NEXT:   ...
la $2, $hello2

@@ -2920,6 +2920,11 @@ bool MipsAsmParser::loadAndAddSymbolAddress(const MCExpr *SymExpr,
(Res.getSymA()->getSymbol().isELF() &&
cast<MCSymbolELF>(Res.getSymA()->getSymbol()).getBinding() ==
ELF::STB_LOCAL);
if (!IsLocalSym && ABI.IsO32()) {
// PrivateGlobalPrefix for O32 is '$', while we support '.L' anyway.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is confusing.

// For O32, "$"-prefixed symbols are recognized as temporary while .L-prefixed symbols are not (PrivateGlobalPrefix is "$"). Recognize ".L" manually.

@@ -0,0 +1,23 @@
# RUN: llvm-mc %s -triple=mips-unknown-linux -show-encoding -mcpu=mips32r2 | \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove -unknown-linux whenever applicable, to emphasize that this is generic ELF behavior.

Copy link

github-actions bot commented Feb 13, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@MaskRay
Copy link
Member

MaskRay commented Feb 13, 2024

Suggested description:

When parsing the la macro, we add a duplicate $ prefix in getOrCreateSymbol,
leading to error: Undefined temporary symbol $$yy for code like:

xx:
	la	$2,$yy
$yy:
	nop

Remove the duplicate prefix.

In addition, recognize .L-prefixed symbols as local for O32.

wzssyqa and others added 4 commits February 13, 2024 21:56
For asm code like:
xx:
	la	$2,$yy
$yy:
	nop

MIPS O32 use `$` as PrivateGlobalPrefix, while another
`$` is added for getOrCreateSymbol, thus:
  error: Undefined temporary symbol $$yy

We also set symbols that starts with `.L` as local for O32.

See: llvm#65020.
@MaskRay MaskRay changed the title MIPS/Asm/O32: Don't add another $ to PrivateGlobal symbol MipsAsmParser/O32: Don't add redundant $ to $-prefixed symbol in the la macro Feb 14, 2024
@MaskRay MaskRay merged commit c007fbb into llvm:main Feb 14, 2024
4 checks passed
llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Feb 15, 2024
…la macro (llvm#80644)

When parsing the `la` macro, we add a duplicate `$` prefix in
`getOrCreateSymbol`,
leading to `error: Undefined temporary symbol $$yy` for code like:

```
xx:
	la	$2,$yy
$yy:
	nop
```

Remove the duplicate prefix.

In addition, recognize `.L`-prefixed symbols as local for O32.

See: llvm#65020.

---------

Co-authored-by: Fangrui Song <[email protected]>
(cherry picked from commit c007fbb)
llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Feb 16, 2024
…la macro (llvm#80644)

When parsing the `la` macro, we add a duplicate `$` prefix in
`getOrCreateSymbol`,
leading to `error: Undefined temporary symbol $$yy` for code like:

```
xx:
	la	$2,$yy
$yy:
	nop
```

Remove the duplicate prefix.

In addition, recognize `.L`-prefixed symbols as local for O32.

See: llvm#65020.

---------

Co-authored-by: Fangrui Song <[email protected]>
(cherry picked from commit c007fbb)
@wzssyqa wzssyqa deleted the local_for_asm branch April 2, 2024 06:52
@pointhex pointhex mentioned this pull request May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mc Machine (object) code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants