[SPARC][IAS] Emit the correct 32-bit ELF machine type #96583

koachan · 2024-06-25T01:56:34Z

Emit the correct machine type when writing out 32-bit ELF objects. This patch is modeled on GCC's behavior:

-m32 emits an object of type EM_SPARC;
-m32 -mcpu=v9 emits EM_SPARC32PLUS (however, see below); and
-m64 emits EM_SPARCV9.

Note:
GNU as doesn't actually support user control of emitted machine type. It will always autodetect the type based on the instruction mix:

If there's a V9 instruction inside, then use EM_SPARC32PLUS; and
Emit EM_SPARC otherwise.

GCC simply happens to try to emit V9 instructions whenever possible with -m32 -mcpu=v9, so there's a high chance that the resulting object file will be of a EM_SPARC32PLUS type, however it is not a guaranteed behavior.

For this patch, I opt for deterministic behavior for simplicity.

Emit the correct machine type when writing out 32-bit ELF objects. This patch is modeled on GCC's behavior: - `-m32` emits an object of type EM_SPARC; - `-m32 -mcpu=v9` emits EM_SPARC32PLUS (however, see below); and - `-m64` emits EM_SPARCV9. Note: GNU as doesn't actually support user control of emitted machine type. It will always autodetect the type based on the instruction mix: - If there's a V9 instruction inside, then use EM_SPARC32PLUS; and - Emit EM_SPARC otherwise. GCC simply happens to try to emit V9 instructions whenever possible with `-m32 -mcpu=v9`, so there's a high chance that the resulting object file will be of a EM_SPARC32PLUS type, however it is not a guaranteed behavior. For this patch, I opt for deterministic behavior for simplicity.

koachan · 2024-06-25T01:57:01Z

CC @glaubitz

llvmbot · 2024-06-25T01:57:06Z

@llvm/pr-subscribers-mc

@llvm/pr-subscribers-backend-sparc

Author: Koakuma (koachan)

Changes

Emit the correct machine type when writing out 32-bit ELF objects. This patch is modeled on GCC's behavior:

-m32 emits an object of type EM_SPARC;
-m32 -mcpu=v9 emits EM_SPARC32PLUS (however, see below); and
-m64 emits EM_SPARCV9.

Note:
GNU as doesn't actually support user control of emitted machine type. It will always autodetect the type based on the instruction mix:

If there's a V9 instruction inside, then use EM_SPARC32PLUS; and
Emit EM_SPARC otherwise.

GCC simply happens to try to emit V9 instructions whenever possible with -m32 -mcpu=v9, so there's a high chance that the resulting object file will be of a EM_SPARC32PLUS type, however it is not a guaranteed behavior.

For this patch, I opt for deterministic behavior for simplicity.

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

3 Files Affected:

(modified) llvm/lib/Target/Sparc/MCTargetDesc/SparcAsmBackend.cpp (+11-6)
(modified) llvm/lib/Target/Sparc/MCTargetDesc/SparcELFObjectWriter.cpp (+8-6)
(modified) llvm/lib/Target/Sparc/MCTargetDesc/SparcMCTargetDesc.h (+2-2)

diff --git a/llvm/lib/Target/Sparc/MCTargetDesc/SparcAsmBackend.cpp b/llvm/lib/Target/Sparc/MCTargetDesc/SparcAsmBackend.cpp
index cb7414fddd29f..046f81363d294 100644
--- a/llvm/lib/Target/Sparc/MCTargetDesc/SparcAsmBackend.cpp
+++ b/llvm/lib/Target/Sparc/MCTargetDesc/SparcAsmBackend.cpp
@@ -132,14 +132,18 @@ namespace {
   class SparcAsmBackend : public MCAsmBackend {
   protected:
     const Target &TheTarget;
+    const MCSubtargetInfo &TheSTI;
     bool Is64Bit;
+    bool HasV9;
 
   public:
-    SparcAsmBackend(const Target &T)
+    SparcAsmBackend(const Target &T, const MCSubtargetInfo &STI)
         : MCAsmBackend(StringRef(T.getName()) == "sparcel"
                            ? llvm::endianness::little
                            : llvm::endianness::big),
-          TheTarget(T), Is64Bit(StringRef(TheTarget.getName()) == "sparcv9") {}
+          TheTarget(T), TheSTI(STI),
+          Is64Bit(StringRef(TheTarget.getName()) == "sparcv9"),
+          HasV9(TheSTI.hasFeature(Sparc::FeatureV9)) {}
 
     unsigned getNumFixupKinds() const override {
       return Sparc::NumTargetFixupKinds;
@@ -340,8 +344,9 @@ namespace {
   class ELFSparcAsmBackend : public SparcAsmBackend {
     Triple::OSType OSType;
   public:
-    ELFSparcAsmBackend(const Target &T, Triple::OSType OSType) :
-      SparcAsmBackend(T), OSType(OSType) { }
+    ELFSparcAsmBackend(const Target &T, const MCSubtargetInfo &STI,
+                       Triple::OSType OSType)
+        : SparcAsmBackend(T, STI), OSType(OSType) {}
 
     void applyFixup(const MCAssembler &Asm, const MCFixup &Fixup,
                     const MCValue &Target, MutableArrayRef<char> Data,
@@ -368,7 +373,7 @@ namespace {
     std::unique_ptr<MCObjectTargetWriter>
     createObjectTargetWriter() const override {
       uint8_t OSABI = MCELFObjectTargetWriter::getOSABI(OSType);
-      return createSparcELFObjectWriter(Is64Bit, OSABI);
+      return createSparcELFObjectWriter(Is64Bit, HasV9, OSABI);
     }
   };
 
@@ -378,5 +383,5 @@ MCAsmBackend *llvm::createSparcAsmBackend(const Target &T,
                                           const MCSubtargetInfo &STI,
                                           const MCRegisterInfo &MRI,
                                           const MCTargetOptions &Options) {
-  return new ELFSparcAsmBackend(T, STI.getTargetTriple().getOS());
+  return new ELFSparcAsmBackend(T, STI, STI.getTargetTriple().getOS());
 }
diff --git a/llvm/lib/Target/Sparc/MCTargetDesc/SparcELFObjectWriter.cpp b/llvm/lib/Target/Sparc/MCTargetDesc/SparcELFObjectWriter.cpp
index f17d3e997452d..bfd71af736231 100644
--- a/llvm/lib/Target/Sparc/MCTargetDesc/SparcELFObjectWriter.cpp
+++ b/llvm/lib/Target/Sparc/MCTargetDesc/SparcELFObjectWriter.cpp
@@ -21,10 +21,12 @@ using namespace llvm;
 namespace {
   class SparcELFObjectWriter : public MCELFObjectTargetWriter {
   public:
-    SparcELFObjectWriter(bool Is64Bit, uint8_t OSABI)
-      : MCELFObjectTargetWriter(Is64Bit, OSABI,
-                                Is64Bit ?  ELF::EM_SPARCV9 : ELF::EM_SPARC,
-                                /*HasRelocationAddend*/ true) {}
+    SparcELFObjectWriter(bool Is64Bit, bool HasV9, uint8_t OSABI)
+        : MCELFObjectTargetWriter(
+              Is64Bit, OSABI,
+              Is64Bit ? ELF::EM_SPARCV9
+                      : (HasV9 ? ELF::EM_SPARC32PLUS : ELF::EM_SPARC),
+              /*HasRelocationAddend*/ true) {}
 
     ~SparcELFObjectWriter() override = default;
 
@@ -146,6 +148,6 @@ bool SparcELFObjectWriter::needsRelocateWithSymbol(const MCValue &,
 }
 
 std::unique_ptr<MCObjectTargetWriter>
-llvm::createSparcELFObjectWriter(bool Is64Bit, uint8_t OSABI) {
-  return std::make_unique<SparcELFObjectWriter>(Is64Bit, OSABI);
+llvm::createSparcELFObjectWriter(bool Is64Bit, bool HasV9, uint8_t OSABI) {
+  return std::make_unique<SparcELFObjectWriter>(Is64Bit, HasV9, OSABI);
 }
diff --git a/llvm/lib/Target/Sparc/MCTargetDesc/SparcMCTargetDesc.h b/llvm/lib/Target/Sparc/MCTargetDesc/SparcMCTargetDesc.h
index a2a9f7474c3f9..63419663b722c 100644
--- a/llvm/lib/Target/Sparc/MCTargetDesc/SparcMCTargetDesc.h
+++ b/llvm/lib/Target/Sparc/MCTargetDesc/SparcMCTargetDesc.h
@@ -34,8 +34,8 @@ MCCodeEmitter *createSparcMCCodeEmitter(const MCInstrInfo &MCII,
 MCAsmBackend *createSparcAsmBackend(const Target &T, const MCSubtargetInfo &STI,
                                     const MCRegisterInfo &MRI,
                                     const MCTargetOptions &Options);
-std::unique_ptr<MCObjectTargetWriter> createSparcELFObjectWriter(bool Is64Bit,
-                                                                 uint8_t OSABI);
+std::unique_ptr<MCObjectTargetWriter>
+createSparcELFObjectWriter(bool Is64Bit, bool HasV9, uint8_t OSABI);
 
 // Defines symbolic names for Sparc v9 ASI tag names.
 namespace SparcASITag {

koachan · 2024-06-25T01:57:27Z

Also, any ideas on how should I go to unit test this change?

glaubitz · 2024-06-28T09:03:56Z

llvm/lib/Target/Sparc/MCTargetDesc/SparcAsmBackend.cpp

        : MCAsmBackend(StringRef(T.getName()) == "sparcel"
                           ? llvm::endianness::little
                           : llvm::endianness::big),
-          TheTarget(T), Is64Bit(StringRef(TheTarget.getName()) == "sparcv9") {}
+          TheTarget(T), TheSTI(STI),
+          Is64Bit(StringRef(TheTarget.getName()) == "sparcv9"),


Unrelated to your change, but I wonder whether checking for the target name being sparcv9 is the proper and robust way to check whether we're on 64-bit SPARC. On Linux, the 64-bit SPARC target is called sparc64 and not sparcv9.

Seems so. In Triple.cpp both sparcv9 and sparc64 resolves to Triple::sparcv9, then in SparcTargetInfo.cpp said triple is registered under the "sparcv9" name.

Now you have STI, you can just get the triple and avoid ugly string comparisons for all these (isArch64Bit and isLittleEndian)

Okay, thanks for the clarification. We're good then. ;-)

Now you have STI, you can just get the triple and avoid ugly string comparisons for all these (isArch64Bit and isLittleEndian)

Whoops, I missed this comment, but fixed too, thanks!

glaubitz · 2024-06-28T09:06:21Z

Looks good to me. Thanks for fixing this!

glaubitz · 2024-06-28T09:07:24Z

Also, any ideas on how should I go to unit test this change?

Maybe using the file command to test for the machine type of a generated object file?

s-barannikov · 2024-06-28T20:57:20Z

Also, any ideas on how should I go to unit test this change?

Look for existing MC tests using llvm-readobj -h.

jrtc27 · 2024-07-01T19:01:06Z

llvm/lib/Target/Sparc/MCTargetDesc/SparcAsmBackend.cpp

@@ -132,14 +132,18 @@ namespace {
  class SparcAsmBackend : public MCAsmBackend {
  protected:
    const Target &TheTarget;


This seems unused beyond in the constructor?

Ah, yeah. Lemme remove it.

glaubitz · 2024-07-01T19:07:12Z

Unit test looks good to me as well. I would prefer squashing the two additional commits into the original commits though since the test belongs to the change.

koachan · 2024-07-02T03:47:28Z

Unit test looks good to me as well. I would prefer squashing the two additional commits into the original commits though since the test belongs to the change.

Yep, it'll be squashed automatically during the merge :)

glaubitz · 2024-07-02T06:59:55Z

Unit test looks good to me as well. I would prefer squashing the two additional commits into the original commits though since the test belongs to the change.

Yep, it'll be squashed automatically during the merge :)

OK, I wasn't aware of that.

s-barannikov

The changes LGTM as a step towards supporting V8+

jrtc27 · 2024-07-02T13:41:49Z

llvm/lib/Target/Sparc/MCTargetDesc/SparcAsmBackend.cpp


  public:
-    SparcAsmBackend(const Target &T)
-        : MCAsmBackend(StringRef(T.getName()) == "sparcel"
+    SparcAsmBackend(const Target &T, const MCSubtargetInfo &STI)


T is unused

Removed, but I guess the one in createSparcAsmBackend should be left as it is?
RegisterMCAsmBackend seems to require a Target parameter present in there...

Yes, that's normal, you just take what you need

koachan · 2024-07-02T14:17:04Z

Git checkout failed as commit a168c9d could not be found in branch koachan:sparc64-v8plus.

Hmm, so how do I rerun the tests? Seems like it cannot find the commit I just pushed?
Or would it be okay if I merge it as it is now?

jrtc27 · 2024-07-02T14:24:18Z

Looks like it got a stale view of the repo for some reason. Probably will work again if you push a dummy commit (git commit --allow-empty?). Buildkite has a Rebuild button but I don't know how you get permission to actually use it.

s-barannikov · 2024-07-02T14:27:47Z

Hmm, so how do I rerun the tests?

Try force pushing or updating the branch (there is a button below CI checks).

Emit the correct machine type when writing out ELF objects. This patch is modeled on GCC's behavior: - `-m32` emits an object of type EM_SPARC; - `-m32 -mcpu=v9` emits EM_SPARC32PLUS (however, see below); and - `-m64` emits EM_SPARCV9. Note that GCC does not guarantee emission of EM_SPARC32PLUS objects, since GNU as doesn't support user control of emitted machine type. It will always autodetect the type based on the instruction mix: - If there's a V9 instruction inside, then emit EM_SPARC32PLUS; and - Emit EM_SPARC otherwise. For LLVM we choose deterministic behavior instead for simplicity.

koachan requested review from jrtc27, brad0, rorth and s-barannikov June 25, 2024 01:56

llvmbot added the backend:Sparc label Jun 25, 2024

glaubitz reviewed Jun 28, 2024

View reviewed changes

Add unit test

d2a1878

llvmbot added the mc Machine (object) code label Jul 1, 2024

Fix typo in test

f3c33c7

jrtc27 reviewed Jul 1, 2024

View reviewed changes

Remove unused members from SparcAsmBackend

4093075

s-barannikov approved these changes Jul 2, 2024

View reviewed changes

Change triple test to use information from STI

a8cf140

jrtc27 reviewed Jul 2, 2024

View reviewed changes

Remove unused Target parameter

a168c9d

Try retriggering CI tests

d2017a5

koachan requested a review from jrtc27 July 3, 2024 03:40

koachan requested a review from s-barannikov July 3, 2024 03:40

s-barannikov approved these changes Jul 3, 2024

View reviewed changes

koachan merged commit fb7f65b into llvm:main Jul 3, 2024
7 checks passed

koachan deleted the sparc64-v8plus branch July 3, 2024 12:14

[SPARC][IAS] Emit the correct 32-bit ELF machine type #96583

[SPARC][IAS] Emit the correct 32-bit ELF machine type #96583

Uh oh!

Conversation

koachan commented Jun 25, 2024

Uh oh!

koachan commented Jun 25, 2024

Uh oh!

llvmbot commented Jun 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

koachan commented Jun 25, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glaubitz commented Jun 28, 2024

Uh oh!

glaubitz commented Jun 28, 2024

Uh oh!

s-barannikov commented Jun 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glaubitz commented Jul 1, 2024

Uh oh!

koachan commented Jul 2, 2024

Uh oh!

glaubitz commented Jul 2, 2024

Uh oh!

s-barannikov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koachan commented Jul 2, 2024

Uh oh!

jrtc27 commented Jul 2, 2024

Uh oh!

s-barannikov commented Jul 2, 2024

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Jun 25, 2024 •

edited

Loading

s-barannikov commented Jun 28, 2024 •

edited

Loading