Why do X86_32TargetMachine and X86_64TargetMachine classes exist?

Project Infrastructure LLVM Dev List Archives
1

These two subclasses of X86TargetMachine are basically identical.

The *only* thing that's different is the constructor. And that *only*
differs in the is64Bit argument that it passes to the X86TargetMachine
constructor. Replacing the hard-coded 'true' or 'false' with
'Triple(TT).getArch()==Triple::x86_64' makes them *actually* identical.

Can we just ditch the subclasses, move the fields and methods that they
share into the X86TargetMachine class, and use that instead? Or am I
missing something?

In the patch I'm about to post to llvm-commits, I *stop* using the
X86_64TargetMachine subclass entirely, and use X86_32TargetMachine for
the 64-bit target. And the 16-bit target too. And nothing seems to have
broken AFAICT...

--- a/lib/Target/X86/X86TargetMachine.cpp
+++ b/lib/Target/X86/X86TargetMachine.cpp
@@ -24,8 +24,9 @@ using namespace llvm;

extern "C" void LLVMInitializeX86Target() {
   // Register the target.
+ RegisterTargetMachine<X86_32TargetMachine> W(TheX86_16Target);
   RegisterTargetMachine<X86_32TargetMachine> X(TheX86_32Target);
- RegisterTargetMachine<X86_64TargetMachine> Y(TheX86_64Target);
+ RegisterTargetMachine<X86_32TargetMachine> Y(TheX86_64Target);
}
@@ -74,7 +75,7 @@ X86_32TargetMachine::X86_32TargetMachine(const Target &T, Stri
                                          const TargetOptions &Options,
                                          Reloc::Model RM, CodeModel::Model CM,
                                          CodeGenOpt::Level OL)
- : X86TargetMachine(T, TT, CPU, FS, Options, RM, CM, OL, false),
+ : X86TargetMachine(T, TT, CPU, FS, Options, RM, CM, OL, Triple(TT).getArch())
     DL(computeDataLayout(*getSubtargetImpl())),
     InstrInfo(*this),

2

Hi David,

AFAIK, the answer is basically “because it’s always been that way.” I seem to recall there were some things that were different (data layout string and such), but that could also be parameterized if it hasn’t been already by the recent refactorings, I suppose.

-Jim

(inline comment below)

These two subclasses of X86TargetMachine are basically identical.

The *only* thing that's different is the constructor. And that *only*
differs in the is64Bit argument that it passes to the X86TargetMachine
constructor. Replacing the hard-coded 'true' or 'false' with
'Triple(TT).getArch()==Triple::x86_64' makes them *actually* identical.

Can we just ditch the subclasses, move the fields and methods that they
share into the X86TargetMachine class, and use that instead? Or am I
missing something?

In the patch I'm about to post to llvm-commits, I *stop* using the
X86_64TargetMachine subclass entirely, and use X86_32TargetMachine for
the 64-bit target. And the 16-bit target too. And nothing seems to have
broken AFAICT...

--- a/lib/Target/X86/X86TargetMachine.cpp
+++ b/lib/Target/X86/X86TargetMachine.cpp
@@ -24,8 +24,9 @@ using namespace llvm;

extern "C" void LLVMInitializeX86Target() {
  // Register the target.
+ RegisterTargetMachine<X86_32TargetMachine> W(TheX86_16Target);
  RegisterTargetMachine<X86_32TargetMachine> X(TheX86_32Target);
- RegisterTargetMachine<X86_64TargetMachine> Y(TheX86_64Target);
+ RegisterTargetMachine<X86_32TargetMachine> Y(TheX86_64Target);
}
@@ -74,7 +75,7 @@ X86_32TargetMachine::X86_32TargetMachine(const Target &T, Stri
                                         const TargetOptions &Options,
                                         Reloc::Model RM, CodeModel::Model CM,
                                         CodeGenOpt::Level OL)
- : X86TargetMachine(T, TT, CPU, FS, Options, RM, CM, OL, false),
+ : X86TargetMachine(T, TT, CPU, FS, Options, RM, CM, OL, Triple(TT).getArch())

Think this is missing a "==Triple::x86_64” at the end, yes?

3

Hi David,

AFAIK, the answer is basically “because it’s always been that way.” I
seem to recall there were some things that were different (data layout
string and such), but that could also be parameterized if it hasn’t
been already by the recent refactorings, I suppose.

It is *all* now parameterized. The classes are identical apart from that
one true/false...

> @@ -74,7 +75,7 @@ X86_32TargetMachine::X86_32TargetMachine(const Target &T, Stri
> const TargetOptions &Options,
> Reloc::Model RM, CodeModel::Model CM,
> CodeGenOpt::Level OL)
> - : X86TargetMachine(T, TT, CPU, FS, Options, RM, CM, OL, false),
> + : X86TargetMachine(T, TT, CPU, FS, Options, RM, CM, OL, Triple(TT).getArch())

Think this is missing a "==Triple::x86_64” at the end, yes?

Nah, I turned that parameter into a Triple::ArchType so that I can pass
x86_16 as an option too. See the patch on llvm-commits which adds the
x86_16 target.

Currently working on a few codegen bugs with building the Linux kernel's
16-bit startup code with 'clang -m16'...

4

Hi David,

AFAIK, the answer is basically “because it’s always been that way.” I
seem to recall there were some things that were different (data layout
string and such), but that could also be parameterized if it hasn’t
been already by the recent refactorings, I suppose.

It is *all* now parameterized. The classes are identical apart from that
one true/false…

Cool. I have no objection to merging them. Seems the right thing to do IMO.

5

I'll sort that out as a preliminary cleanup before the addition of
x86_16 target then; thanks.

That way I don't have to add a *third* gratuitous identical class :slight_smile:

6

Looks like this...

0001-x86-Kill-gratuitous-X86_-32-64-TargetMachine-subclas.patch (9.71 KB)

0002-Add-x86_16-target.patch (12.3 KB)

7

LGTM

-Jim