[lld] Remove duplicate class definitions of ELF LinkingContexts

[filcab]

I have some nits.
Sorry about getting some of the surrounding lines that I ended up not commenting on the other patch, but I figured, since it's from the same revision and you're changing that code… Might as well comment on it.

Thanks for fixing this!

include/lld/ReaderWriter/ELFTargets.h

18

Why LLVM_TARGET? What does this have to do (directly) with llvm and its (enabled?) targets?
Why not ELF_TARGET (or ELF_ARCH), or something like that, since it's about targets supported for linking ELF files?

lib/Driver/GnuLdDriver.cpp

339

IIUC, nothing in lld depends on llvm's enabled targets (except some MIPS tests, IIRC). Why wouldn't this just depend on “targets lld can handle”?

341

Why not switch on the Triple, and call the appropriate function (which wouldn't have the Triple check, but (possibly) an assert), instead of doing the whole “try to create*()” dance?

344

We have this list of LLVM_TARGET expansions at least twice. Do we want something like a .def file?

[• shankar.easwaran]

filcab,

We need lld to be tightly integrated with LLVM targets, these are the reasons I have in mind :

• targets need to get instruction eencoding recorded in the Td file. If you see the Hexagon target in lld we have one huge file that contains all the encodings which is just duplicating informattoo in the Td files. I believe MIPS also needs this encoding information.
• enable better diagnostics with encoding information.

It's also needed in the future for LTO purposes to invoke the code generator from lld after reading bit code files.

I would like to propose LLD_TARGET_<flavor> as we have multiple flavors that may support the same target in future. Other proposals are welcome too.

[garious]

LLD is in an awkward limbo state where it acts like it's a first-class member of the LLVM tree and also acts like an independent project that only depends on LLVM for its Support and CMake libraries. If it wants to be the former, LLD should respect LLVM_TARGETS_TO_BUILD. If the latter, it doesn't matter and could have its own Targets.def. But if that's the case, LLD should have a standalone CMake build.

My preference *was* to treat LLD as if it were part of the LLVM tree, with the hope that LLD would be integrated. I like the idea that the LLVM repo would be responsible for everything from bitcode to linked executables instead of everything from bitcode to object files. I sent an email to llvmdev to see if others felt the same way, but got very little feedback. So now I'm moving under the assumption that LLD will be a standalone project for the foreseeable future. Surprisingly, my attempt to revive the standalone CMake build is being ignored. Clarification would be much appreciated.

lib/Driver/GnuLdDriver.cpp

339

Because it could be both "targets lld can handle" and "targets enabled by the llvm build". You can mix the two by adding a 'create' function for each llvm-supported target and returning nullptr to indicate "unsupported by lld".

341

So that we can enable only select targets via LLVM_TARGETS_TO_BUILD.

344

Sure, I'll add a .def file.

[hfinkel]

• Original Message -----

From: "Greg Fitzgerald" <[e-mail]>
To: [e-mail], [e-mail]
Cc: "shankar kalpathi easwaran" <[e-mail]>, "filcab+llvm phabricator"
<[e-mail]>, [e-mail]
Sent: Saturday, February 21, 2015 12:34:58 PM
Subject: Re: [PATCH] [lld] Remove duplicate class definitions of ELF LinkingContexts

LLD is in an awkward limbo state where it acts like it's a
first-class member of the LLVM tree and also acts like an
independent project that only depends on LLVM for its Support and
CMake libraries. If it wants to be the former, LLD should respect
LLVM_TARGETS_TO_BUILD. If the latter, it doesn't matter and could
have its own Targets.def. But if that's the case, LLD should have a
standalone CMake build.

My preference *was* to treat LLD as if it were part of the LLVM tree,
with the hope that LLD would be integrated.

My understanding, as a general member of the LLVM community, was that the eventual goal was the integrate lld into the rest of the system. For a long time, this was not possible because lld used C++11 while LLVM itself did not. This is obviously not true any more, and I think that if there are further reasons to continue the current level of separation, those should be well explained.

I like the idea that
the LLVM repo would be responsible for everything from bitcode to
linked executables instead of everything from bitcode to object
files.

I also like this idea.

-Hal

I sent an email to llvmdev to see if others felt the same
way, but got very little feedback. So now I'm moving under the
assumption that LLD will be a standalone project for the foreseeable
future. Surprisingly, my attempt to revive the standalone CMake
build is being ignored. Clarification would be much appreciated.

REPOSITORY

rL LLVM

Comment at: lib/Driver/GnuLdDriver.cpp:339
@@ -338,2 +338,3 @@

std::unique_ptr<ELFLinkingContext> p;
// FIXME: #include "llvm/Config/Targets.def"

+#define LLVM_TARGET(name) \

filcab wrote:

IIUC, nothing in lld depends on llvm's enabled targets (except some
MIPS tests, IIRC). Why wouldn't this just depend on “targets lld
can handle”?

Because it could be both "targets lld can handle" and "targets
enabled by the llvm build". You can mix the two by adding a
'create' function for each llvm-supported target and returning
nullptr to indicate "unsupported by lld".

Comment at: lib/Driver/GnuLdDriver.cpp:341
@@ -342,1 +340,3 @@
+#define LLVM_TARGET(name) \
+ if ((p = elf::createnameLinkingContext(triple))) return p;

LLVM_TARGET(AArch64)

---

filcab wrote:

Why not switch on the Triple, and call the appropriate function
(which wouldn't have the Triple check, but (possibly) an assert),
instead of doing the whole “try to create*()” dance?

So that we can enable only select targets via LLVM_TARGETS_TO_BUILD.

Comment at: lib/Driver/GnuLdDriver.cpp:344
@@ -343,2 +343,2 @@

LLVM_TARGET(ARM)
LLVM_TARGET(Hexagon)

---

filcab wrote:

We have this list of LLVM_TARGET expansions at least twice. Do we
want something like a .def file?

Sure, I'll add a .def file.

http://reviews.llvm.org/D7807

EMAIL PREFERENCES

http://reviews.llvm.org/settings/panel/emailpreferences/

---

llvm-commits mailing list
[e-mail]
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

[filcab]

Hi Shankar,

On Sat, Feb 21, 2015 at 9:55 AM, Shankar Easwaran <[e-mail]> wrote:
filcab,

We need lld to be tightly integrated with LLVM targets, these are the reasons I have in mind :

• targets need to get instruction eencoding recorded in the Td file. If you see the Hexagon target in lld we have one huge file that contains all the encodings which is just duplicating informattoo in the Td files. I believe MIPS also needs this encoding information.

Is this about the code like what's in http://llvm.org/klaus/lld/blob/master/lib/ReaderWriter/ELF/Hexagon/HexagonTargetHandler.cpp#L-54 (and other files for other archs)?

From talking to Michael, I got the impression that it's such a small amount of code (that will basically never change), that it's not worth it to have lld depend on all the target infrastructure for no or very little gains.

• enable better diagnostics with encoding information.

What kinds of diagnostics, for what kinds of errors?

It's also needed in the future for LTO purposes to invoke the code generator from lld after reading bit code files.

I don't understand this. lld should be able to use the lto library without caring about which targets are enabled (as long as lld can handle the triple it wants, of course). It's “just”: “here's bitcode. Give me machine code”. And then finish the link. No target things should leak out of the lto library. Or is this wrong?
The way I see it is: lld depends (directly) on the lto library. The lto library will depend on the targets, but lld won't depend on them directly.

I would like to propose LLD_TARGET_<flavor> as we have multiple flavors that may support the same target in future. Other proposals are welcome too.

Doesn't sound bad to me :)

Thanks,

Filipe

(edited: tried to get the email's full contents on phabricator)

[filcab]

I'm all for integrating lld into llvm (I can accept either version), but it doesn't make sense to start having some scattered pieces of code to handle it, if there has been no decision on integrating lld into llvm.

lib/Driver/GnuLdDriver.cpp

339

But we're not doing that now. lld fully implements everything, without caring about which llvm targets are enabled, which means we don't have the case where we want to selectively compile lld's support depending on what support is compiled in llvm).

I would prefer to have a clean implementation now and, if we ever change and start depending on the llvm targets directly, change to another clean implementation. Right now, having those tests in the linking contexts seem useless (I might be missing something, of course).

341

Even if that's a future feature, it still makes the name misleading, right now.

[shankarke]

Filcab, you got the wrong file, please look at this file https://github.com/llvm-mirror/lld/blob/master/lib/ReaderWriter/ELF/Hexagon/HexagonEncodings.h (this is pretty common with scatter architectures, where all the information is present in the td file and you are trying to duplicate this information in target specific code in other tools, this is not clean).

In terms of diagnostics, I would like to atleast figure out the instruction after being relocated, so possibly get a way to figure out how instructions are changing with respect to relocations.

You are right, generally you dont need LTO to know about the LLVM target, it just needs the right target riple to be set and used.

[• shankar.easwaran]

With hexagon the relocation encoding depends on the instruction. So when the relocation need to be applied the encoding mask has to be figured out from the instruction how it encodes the immediate bits.

In the case of LTO, it was needed that the linker needed information about target specific behavior to handle specific common symbols. As you mentioned generally it's not needed.

include/lld/ReaderWriter/ELFTargets.h

 namespace lld {
 namespace elf {
 
-#define LLVM_TARGET(TargetName) \
-  class TargetName##LinkingContext final : public ELFLinkingContext { \
-  public: \
-    static std::unique_ptr<ELFLinkingContext> create(llvm::Triple); \
-  };
+#define LLVM_TARGET(name) \
+  std::unique_ptr<ELFLinkingContext> create##name##LinkingContext(llvm::Triple);
 
 // FIXME: #include "llvm/Config/Targets.def"
 LLVM_TARGET(AArch64)

lib/Driver/GnuLdDriver.cpp

 GnuLdDriver::createELFLinkingContext(llvm::Triple triple) {
   std::unique_ptr<ELFLinkingContext> p;
   // FIXME: #include "llvm/Config/Targets.def"
-#define LLVM_TARGET(targetName) \
-  if ((p = elf::targetName##LinkingContext::create(triple))) return p;
+#define LLVM_TARGET(name) \
+  if ((p = elf::create##name##LinkingContext(triple))) return p;
   LLVM_TARGET(AArch64)
   LLVM_TARGET(ARM)
   LLVM_TARGET(Hexagon)

lib/ReaderWriter/ELF/AArch64/AArch64LinkingContext.h

 
 class AArch64LinkingContext final : public ELFLinkingContext {
 public:
-  static std::unique_ptr<ELFLinkingContext> create(llvm::Triple);
   AArch64LinkingContext(llvm::Triple);
 
   void addPasses(PassManager &) override;

lib/ReaderWriter/ELF/AArch64/AArch64LinkingContext.cpp

 #include "AArch64LinkingContext.h"
 #include "AArch64RelocationPass.h"
 #include "AArch64TargetHandler.h"
+#include "lld/ReaderWriter/ELFTargets.h"
 
 using namespace lld;
 
 std::unique_ptr<ELFLinkingContext>
-elf::AArch64LinkingContext::create(llvm::Triple triple) {
+elf::createAArch64LinkingContext(llvm::Triple triple) {
   if (triple.getArch() == llvm::Triple::aarch64)
     return std::unique_ptr<ELFLinkingContext>(
              new elf::AArch64LinkingContext(triple));

lib/ReaderWriter/ELF/ARM/ARMLinkingContext.h

 
 class ARMLinkingContext final : public ELFLinkingContext {
 public:
-  static std::unique_ptr<ELFLinkingContext> create(llvm::Triple);
   ARMLinkingContext(llvm::Triple);
 
   void addPasses(PassManager &) override;

lib/ReaderWriter/ELF/ARM/ARMLinkingContext.cpp

 #include "ARMLinkingContext.h"
 #include "ARMRelocationPass.h"
 #include "ARMTargetHandler.h"
+#include "lld/ReaderWriter/ELFTargets.h"
 
 using namespace lld;
 using namespace lld::elf;
 
 std::unique_ptr<ELFLinkingContext>
-elf::ARMLinkingContext::create(llvm::Triple triple) {
+elf::createARMLinkingContext(llvm::Triple triple) {
   if (triple.getArch() == llvm::Triple::arm)
     return std::unique_ptr<ELFLinkingContext>(
              new elf::ARMLinkingContext(triple));

lib/ReaderWriter/ELF/Hexagon/HexagonLinkingContext.h

 
 class HexagonLinkingContext final : public ELFLinkingContext {
 public:
-  static std::unique_ptr<ELFLinkingContext> create(llvm::Triple);
   HexagonLinkingContext(llvm::Triple triple);
 
   void addPasses(PassManager &) override;

lib/ReaderWriter/ELF/Hexagon/HexagonLinkingContext.cpp

 
 #include "HexagonLinkingContext.h"
 #include "HexagonTargetHandler.h"
+#include "lld/ReaderWriter/ELFTargets.h"
 
 using namespace lld::elf;
 
 std::unique_ptr<lld::ELFLinkingContext>
-HexagonLinkingContext::create(llvm::Triple triple) {
+lld::elf::createHexagonLinkingContext(llvm::Triple triple) {
   if (triple.getArch() == llvm::Triple::hexagon)
     return std::unique_ptr<lld::ELFLinkingContext>(
              new HexagonLinkingContext(triple));

lib/ReaderWriter/ELF/Mips/MipsLinkingContext.h

 
 class MipsLinkingContext final : public ELFLinkingContext {
 public:
-  static std::unique_ptr<ELFLinkingContext> create(llvm::Triple);
   MipsLinkingContext(llvm::Triple triple);
 
   uint32_t getMergedELFFlags() const;

lib/ReaderWriter/ELF/Mips/MipsLinkingContext.cpp

 #include "MipsLinkingContext.h"
 #include "MipsRelocationPass.h"
 #include "MipsTargetHandler.h"
+#include "lld/ReaderWriter/ELFTargets.h"
 
 using namespace lld;
 using namespace lld::elf;
 
 std::unique_ptr<ELFLinkingContext>
-MipsLinkingContext::create(llvm::Triple triple) {
+lld::elf::createMipsLinkingContext(llvm::Triple triple) {
   if (triple.getArch() == llvm::Triple::mipsel ||
       triple.getArch() == llvm::Triple::mips64el)
     return std::unique_ptr<ELFLinkingContext>(new MipsLinkingContext(triple));

lib/ReaderWriter/ELF/X86/X86LinkingContext.h

 namespace elf {
 class X86LinkingContext final : public ELFLinkingContext {
 public:
-  static std::unique_ptr<ELFLinkingContext> create(llvm::Triple);
   X86LinkingContext(llvm::Triple);
 
   /// \brief X86 has only two relative relocation

lib/ReaderWriter/ELF/X86/X86LinkingContext.cpp

 #include "X86LinkingContext.h"
 #include "X86TargetHandler.h"
 #include "lld/Core/LLVM.h"
+#include "lld/ReaderWriter/ELFTargets.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/Support/ErrorOr.h"
 
 using namespace lld;
 
 std::unique_ptr<ELFLinkingContext>
-elf::X86LinkingContext::create(llvm::Triple triple) {
+elf::createX86LinkingContext(llvm::Triple triple) {
   if (triple.getArch() == llvm::Triple::x86)
     return std::unique_ptr<ELFLinkingContext>(
              new elf::X86LinkingContext(triple));

lib/ReaderWriter/ELF/X86_64/X86_64LinkingContext.h

 
 class X86_64LinkingContext final : public ELFLinkingContext {
 public:
-  static std::unique_ptr<ELFLinkingContext> create(llvm::Triple);
   X86_64LinkingContext(llvm::Triple);
 
   void addPasses(PassManager &) override;

lib/ReaderWriter/ELF/X86_64/X86_64LinkingContext.cpp

 #include "X86_64LinkingContext.h"
 #include "X86_64TargetHandler.h"
 #include "X86_64RelocationPass.h"
+#include "lld/ReaderWriter/ELFTargets.h"
 
 using namespace lld;
 
 std::unique_ptr<ELFLinkingContext>
-elf::X86_64LinkingContext::create(llvm::Triple triple) {
+elf::createX86_64LinkingContext(llvm::Triple triple) {
   if (triple.getArch() == llvm::Triple::x86_64)
     return std::unique_ptr<ELFLinkingContext>(
              new elf::X86_64LinkingContext(triple));