llvm dev - Jan 2014 - [LLVMdev] Why do X86_32TargetMachine and X86_64TargetMachine classes exist?

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),


-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse at intel.com                              Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 5745 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140106/ab26c06b/attachment.bin>

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)

On Jan 6, 2014, at 7:18 AM, David Woodhouse <dwmw2 at infradead.org>
wrote:
> 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?
>     DL(computeDataLayout(*getSubtargetImpl())),
>     InstrInfo(*this),
> 
> 
> -- 
> David Woodhouse                            Open Source Technology Centre
> David.Woodhouse at intel.com                              Intel Corporation
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

On Mon, 2014-01-06 at 14:23 -0800, Jim Grosbach wrote:
> 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'...

-- 
dwmw2
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 5745 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140106/c5313a98/attachment.bin>