deglobalizing TargetOptions

I'm running LLVM under threadsanitizer
(http://code.google.com/p/data-race-test/) to find and remove races in
a larger program that uses LLVM as a library. One of the things that I
found is that the TargetOptions are all global; you could create a
TargetMachine targeting ARM and X86 in two threads, but they both have
to share the same FloatABIType setting.

This is silly, and I plan to fix it by moving all the globals in
TargetOptions into getters and setters on TargetMachine. This means
that the TargetMachine object is going to get a bit bigger to
accommodate, but at least two users of llvm linked into one program
won't step on each other. The existing command-line options will move
from TargetMachine into llc which will use the same setters. This has
the side-effect that "clang -mllvm" won't work for them any more.

If you think this change should not happen, or should happen
differently, please let me know.

Nick

I'm running LLVM under threadsanitizer
(http://code.google.com/p/data-race-test/) to find and remove races in
a larger program that uses LLVM as a library. One of the things that I
found is that the TargetOptions are all global; you could create a
TargetMachine targeting ARM and X86 in two threads, but they both have
to share the same FloatABIType setting.

This is silly, and I plan to fix it by moving all the globals in
TargetOptions into getters and setters on TargetMachine. This means
that the TargetMachine object is going to get a bit bigger to
accommodate, but at least two users of llvm linked into one program
won't step on each other.

Sounds great.

The existing command-line options will move
from TargetMachine into llc which will use the same setters. This has
the side-effect that "clang -mllvm" won't work for them any more.

Sounds fine.

Instead of adding a bunch of instance variables (+ getters/setters) into TargetMachine, why not make TargetOptions be a class, and have TM contain an instance of it?

-Chris

I'm running LLVM under threadsanitizer
(http://code.google.com/p/data-race-test/) to find and remove races in
a larger program that uses LLVM as a library. One of the things that I
found is that the TargetOptions are all global; you could create a
TargetMachine targeting ARM and X86 in two threads, but they both have
to share the same FloatABIType setting.

This is silly, and I plan to fix it by moving all the globals in
TargetOptions into getters and setters on TargetMachine. This means
that the TargetMachine object is going to get a bit bigger to
accommodate, but at least two users of llvm linked into one program
won't step on each other.

Sounds great.

The existing command-line options will move
from TargetMachine into llc which will use the same setters. This has
the side-effect that "clang -mllvm" won't work for them any more.

Sounds fine.

Thanks Chris!

Instead of adding a bunch of instance variables (+ getters/setters) into TargetMachine, why not make TargetOptions be a class, and have TM contain an instance of it?

That works too, it makes little difference to me. One reason is that
most references to these globals are inside classes that derive from
TargetMachine so I wouldn't need to touch those lines. The second
reason is that I'm going to turn "bool Foo" into "unsigned Foo : 1"
and would like that to pack in with the 6 variables that are currently
members of the TargetMachine.

How about I just move the 6 existing flags into TargetOptions and even
remove their getters/setters. I think that'll be cleanest overall.

Nick

I'd really prefer these to be in a separate class: TM is just too huge and full of random things already. I'm not concerned about the actual sizeof(TargetMachine), so reusing a few bits in it shouldn't matter.

-Chris

Chris Lattner wrote:

Instead of adding a bunch of instance variables (+ getters/setters) into TargetMachine, why not make TargetOptions be a class, and have TM contain an instance of it?

That works too, it makes little difference to me. One reason is that
most references to these globals are inside classes that derive from
TargetMachine so I wouldn't need to touch those lines. The second
reason is that I'm going to turn "bool Foo" into "unsigned Foo : 1"
and would like that to pack in with the 6 variables that are currently
members of the TargetMachine.

How about I just move the 6 existing flags into TargetOptions and even
remove their getters/setters. I think that'll be cleanest overall.

I'd really prefer these to be in a separate class: TM is just too huge and full of random things already. I'm not concerned about the actual sizeof(TargetMachine), so reusing a few bits in it shouldn't matter.

Sounds good, committed in r145714 and a number of followup patches for the frontends.

Unfortunately there's two failing buildbots left, both building llvm-gcc on ARM:
http://lab.llvm.org:8011/builders/llvm-gcc-i686-pc-linux-gnu-cross-arm-eabi-soft-float/
http://lab.llvm.org:8011/builders/llvm-gcc-i686-pc-linux-gnu-cross-arm-eabi-hard-float/

Galina, those are yours. I don't understand why the buildbots are failing (where are they putting -soft-float and -float-abi on cc1's command line?) but I also don't understand why we're still building llvm-gcc. I really only want to bring this up because I'm worried that the same bug might be shared with dragonegg. Is this something you can investigate?

Nick

Hi Nick,
`

Galina, those are yours. I don't understand why the buildbots are
failing (where are they putting -soft-float and -float-abi on cc1's
command line?) but I also don't understand why we're still building
llvm-gcc. I really only want to bring this up because I'm worried that
the same bug might be shared with dragonegg. Is this something you can
investigate?

Inside gcc/config/arm/arm.h you'll find:

#define LLVM_SET_MACHINE_OPTIONS(argvec) \
  if (TARGET_SOFT_FLOAT) \
    argvec.push_back("-soft-float"); \
  if (TARGET_HARD_FLOAT_ABI) \
    argvec.push_back("-float-abi=hard"); \
  if (flag_mkernel || flag_apple_kext) { \
    argvec.push_back("-arm-long-calls"); \
    argvec.push_back("-arm-strict-align"); \
  }

You can find something similar inside gcc/config/i386/i386.h and rs6000h.