Fix for non-standard variable length array

In lib/CodeGen/LiveVariables.cpp a variable length array is created. Since this is not standard C++, it won't compile with Visual Studio (for interested readers, there is a discussion of why the C99 style variable length array is a bad idea in C++ here: http://www.cuj.com/documents/s=8191/cuj0209stroustr/) I have made a patch that uses 'new' and 'delete' instead - if this is unacceptable for performance reasons, I suggest just choosing some fixed size for the arrays (128 for example) and asserting that the actual number of registers is smaller than this. Patch follows.

On a side note I can now run the fibonacci example with the x86 backend compiled with Visual C -- For fun I compiled the fibonacci function also with Visual C with max. optimizes, and the LLVM compiled version is slightly faster! I'm impressed! More patches will follow, sorry for flooding...

m.

There was a similar problem some time ago, and was resolved with alloca. I think it's a better solution to use the stack instead of the heap...

Paolo Invernizzi wrote:

There was a similar problem some time ago, and was resolved with alloca. I think it's a better solution to use the stack instead of the heap...

I tend to agree, but the constructors won't get called if it's an object array -- anyway, this particular case there was no objects, just pointers and bools so alloca should be fine. I'll leave it to Chris Lattner to decide if my patch goes in or not...

To reduce the number of mails, I also include my next patch -- X86 specific code and inline assembly for Visual C, unfortunately I had to use the nasty IncludeFile trick again to get the linker to work..

m.

diff.txt (3.49 KB)

There was a similar problem some time ago, and was resolved with alloca. I think it's a better solution to use the stack instead of the heap...

Actually, I looked into this and alloca is not standard C++ since it can create problems for the exception handling (although it compiles fine with Visual C). This is probably why there have been compilation problems on some platforms (see http://mail.cs.uiuc.edu/pipermail/llvmdev/2004-September/001992.html) -- A better alternative is maybe to use the std::vector instead of a VLA.

m.

I submitted a patch with a std::vector, but was commited as alloca :stuck_out_tongue_winking_eye:

Morten,

I have committed all your patches except this one. Chris will need to
adjudicate this as changing the allocation can (potentially) change the
behavior and/or performance of the code.

Patches are here:

http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20041018/019488.html
http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20041018/019489.html
http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20041018/019490.html
http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20041018/019491.html

Reid.

Paolo Invernizzi wrote:

I submitted a patch with a std::vector, but was commited as alloca :stuck_out_tongue_winking_eye:

for the other problem?

I made a nice clean patch using std::vector in LiveVariables.cpp now, which I include with this message... By the way, where do you submit the patches if not to the mailing list? I thought I should post here so Paolo and others interested in the porting effort can apply them locally without waiting for them to be accepted in the main tree, but maybe there is a better way?

m.

diff.txt (1.92 KB)

Paolo Invernizzi wrote:

I submitted a patch with a std::vector, but was commited as alloca :stuck_out_tongue_winking_eye:

for the other problem?

Yep, there was another C99 array in the sources

I made a nice clean patch using std::vector in LiveVariables.cpp now, which I include with this message... By the way, where do you submit the patches if not to the mailing list?

I submitted patches to the ML, but usually grouping them together "by cases"... missing header, missing std:: prefixes and so on...

I thought I should post here so Paolo and others interested in the porting effort can apply them locally without waiting for them to be accepted in the main tree, but maybe there is a better way?

I would like to have the time to try out your recent patches... but I pretty busy ;-((

You're doing it the right way. Patches should be submitted to LLVMdev so
everyone can see them and act accordingly. We try to be as responsive
as possible on patches so the main tree is not lagging behind your local
tree.

Reid.

In this case, as Paolo pointed out, I'd much rather use an alloca instead
of a vector. None of the LLVM-to-LLVM transformations throw exceptions so
that's not a worry.

Thanks for all of the great patches!

-Chris

I applied most of this here:
http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20041018/019493.html

Can you explain what goes wrong without the stub? It's the only part that
I didn't apply.

Thanks!

-Chris

Chris Lattner wrote:

Can you explain what goes wrong without the stub? It's the only part that
I didn't apply.

The X86 backend doesn't get registered since there are no references to symbols in X86TargetMachine the object file is never pulled in from the library I create, and thus the static intializer for the RegisterTarget is never called...

   RegisterTarget<X86TargetMachine> X("x86", " IA-32 (Pentium and above)");

I've tried some other ways to resolve it, but so far I've had no luck .. any suggestions are appreciated, as this is a problem in other places as well - and I'm not happy with the IncludeFile hack, I can well understand why you didn't want to apply it.

m.

Chris Lattner wrote:
> Can you explain what goes wrong without the stub? It's the only part that
> I didn't apply.

The X86 backend doesn't get registered since there are no references to
symbols in X86TargetMachine the object file is never pulled in from the
library I create, and thus the static intializer for the RegisterTarget
is never called...

I'm not sure how your patch fixes it though. The only references added by
your change would be within the X86 library. How does this change the
situation?

   RegisterTarget<X86TargetMachine> X("x86", " IA-32 (Pentium and above)");

I've tried some other ways to resolve it, but so far I've had no luck ..
   any suggestions are appreciated, as this is a problem in other places
as well - and I'm not happy with the IncludeFile hack, I can well
understand why you didn't want to apply it.

One of the funny things about the LLVM Unix build is that we can build
libraries in three different ways: as .a files, as .so files, and as .o
files. The IncludeFile hack is used for libraries compmiled to .a files,
to make sure that the library is actually linked in. The other two types
of library (X86 is a .o file version) do not have this problem: they
"library" either gets linked in as a unit or not.

I'm not sure if VC has something like this, but I strongly suspect it
does. The trick is getting libx86 to compile as a single unit that is
forced to link in.

-Chris

It depends on whether the linker can "relink" a set of object files into
another object file instead of an executable.

Morten, what you want to set up is the ability to do something like:

link a.obj b.obj c.obj /out abc.obj

(pardon my lack of knowledge on the MS linker)

note that the output file is .obj not .exe. If the linker can do that,
then you want to build every library this way. If the "Makefile" in a
lib directory says "BUILD_ARCHIVE = 1" then you also want to build a
regular library (.lib). If it says "SHARED_LIBRARY = 1" then you want to
build a DLL from it (might not be possible, DLLs have some code
requirements we don't support, I think .. e.g. export/import ).

Reid.

Chris Lattner wrote:

Can you explain what goes wrong without the stub? It's the only part that
I didn't apply.

The X86 backend doesn't get registered since there are no references to
symbols in X86TargetMachine the object file is never pulled in from the
library I create, and thus the static intializer for the RegisterTarget
is never called...

I'm not sure how your patch fixes it though. The only references added by
your change would be within the X86 library. How does this change the
situation?

Because I'm including the X86TargetMachine header from the application (Fibonacci.cpp) .. I know, I know -- but really, I tried everything to get the linker to cooperate. I can only generate static libraries (.lib) and dynamic libraries (.dll) - there is an option to the linker to force symbol references, but it has to be put in the final link for the application and then the object X which registers the target has to be put in the global namespace with an 'extern "C"' in front to know it's name. It's even worse in my opinion.

One of the funny things about the LLVM Unix build is that we can build
libraries in three different ways: as .a files, as .so files, and as .o
files. The IncludeFile hack is used for libraries compmiled to .a files,
to make sure that the library is actually linked in. The other two types
of library (X86 is a .o file version) do not have this problem: they
"library" either gets linked in as a unit or not.

I'm not sure if VC has something like this, but I strongly suspect it
does. The trick is getting libx86 to compile as a single unit that is
forced to link in.

maybe it's possible to put many objects together into one with the visual studio compiler also, but I don't really know how to do it... especially not if you want to stick with the normal .sln/.vcproj build system...

m.

Reid Spencer wrote:

It depends on whether the linker can "relink" a set of object files into
another object file instead of an executable.

Now that I understand better the problem, I have done some research. The conclusion is that the Visual Studio linker can _not_ do this.

On a side note, I'm not really a big fan of static initializers in general since you never know which order they are going to be called in. That means you cannot rely on _anything_ being initialized beyond the current object, and this can lead to some extremely hard to find bugs...

m.

> I'm not sure how your patch fixes it though. The only references added by
> your change would be within the X86 library. How does this change the
> situation?

Because I'm including the X86TargetMachine header from the application
(Fibonacci.cpp) .. I know, I know -- but really, I tried everything to
get the linker to cooperate.

Ugh! I'm sure you can appreciate the fact that we can't do such a thing.
The X86 backend isn't even linked into the JIT on hosts that are not X86,
and the JIT even allows for target support to be dynamically loaded:

lli -load yourchip.so foo.bc

Adding X86 specific stuff to clients is not going to work.

I can only generate static libraries (.lib)
and dynamic libraries (.dll) - there is an option to the linker to force
symbol references, but it has to be put in the final link for the
application and then the object X which registers the target has to be
put in the global namespace with an 'extern "C"' in front to know it's
name. It's even worse in my opinion.

I think forcing a reference is ok. If you need to add an extern "C"
symbol, that shouldn't be a problem. Alternatively, compiling the X86
support as a dll would also work, have you tried that?

-Chris

Alternatively, compiling the X86
support as a dll would also work, have you tried that?

Why is it supposed to work? Linking with an import library will add all its symbols? I was not expecting a different behavior between linking against a static and a dynamic library...

Alternatively, compiling the X86
support as a dll would also work, have you tried that?

Why is it supposed to work? Linking with an import library will add all its symbols? I was not expecting a different behavior between linking against a static and a dynamic library...

It would work because the dll would call the static initializers -- within the dll there are references between the objects, and calling the DllMain will require the static initializers to be called.

However there are huge problems with creating a dll, since every symbol used in the dll which comes from the outside have to be imported from other dlls so everything would have to be converted to dlls and having proper __declspec(dllexport) / __declspec(dllimport) everywhere. I think it's just not feasible.

My only remaining idea is to create a file which #includes all the .cpp files for the x86 target to create a single object...

m.