This is an archive of the discontinued LLVM Phabricator instance.

Move types in the LLVM C API from Core to Support
ClosedPublic

Authored by deadalnix on Oct 4 2015, 4:26 PM.

Details

Summary

Becuase these type were on Core, it required core to be included all over the place, notably in IR/Value.h and IR/Type.h . As a result, any change in Core.h require to recompile LLVM almost completely, which is painful.

Moving them to Support.h allow for most of LLVM to not depend on Core.h , saving a lot of compilation time.

There is no functionality change, only reorganisation of header to save compilation time.

Diff Detail

Event Timeline

deadalnix updated this revision to Diff 36475.Oct 4 2015, 4:26 PM
deadalnix retitled this revision from to Move types in the LLVM C API from Core to Support.
deadalnix updated this object.
deadalnix added a subscriber: llvm-commits.

Reduced the compilation needs when touching Core.h from more than 706 down to 94.

jyknight edited edge metadata.Oct 5 2015, 12:40 PM

I'm not sure this factoring actually makes sense: Support.h looks like it's intended to be exposing functionality in the lib/Support/*.cpp files, right? So putting these types into its header file would be misplacing them.

@jyknight I put it there because there was already some common facilities in it, like LLVMBool and it is included across the C API. I can create a new header file if that is necessary (actually, that makes a lot of sense).

deadalnix updated this revision to Diff 36554.Oct 5 2015, 2:30 PM
deadalnix edited edge metadata.

Lift types declarations in Types.h

deadalnix updated this revision to Diff 36562.Oct 5 2015, 2:49 PM

Also split ErrorHandling.h so that we don't need to recompile tablegen at any change of the C API.

This seems sane to me. Of course, user code which said only:

#include "llvm-c/BitReader.h"

and then depended upon getting the functions in llvm-c/Core.h transitively included will need to add a #include "llvm-c/Core.h", but that seems perfectly fine to me. My go-to examples of extremely stable libraries, glibc and libstdc++, make that kind of change all the time, for example.

Eric, do you object?

jyknight added inline comments.Oct 14 2015, 7:55 AM
include/llvm-c/Core.h
19

Would be good to add:
#include "llvm-c/ErrorHandling.h"
here, to continue defining those functions when including this header, so users don't need to do version-conditions to include a brand new header for existing functions.

This seems sane to me. Of course, user code which said only:

#include "llvm-c/BitReader.h"

and then depended upon getting the functions in llvm-c/Core.h transitively included will need to add a #include "llvm-c/Core.h", but that seems perfectly fine to me. My go-to examples of extremely stable libraries, glibc and libstdc++, make that kind of change all the time, for example.

Eric, do you object?

Doesn't this break the source level compatibility that was a part of what people were talking about in the C API thread?

Personally? No, I don't object and am in favor, but I don't feel comfortable approving it until we get some of the other stuff resolved.

-eric

@echristo I do understand that people are more attached to ABI compatibility than source compatibility.

deadalnix updated this revision to Diff 38680.Oct 28 2015, 12:51 PM
deadalnix edited edge metadata.

Rebase and fix the Orc C API test case.

deadalnix updated this revision to Diff 42377.Dec 9 2015, 9:24 PM

ping ! @echristo , has the C API topic made any progress ?

deadalnix updated this revision to Diff 42883.Dec 15 2015, 11:33 AM

Include include/llvm-c/ErrorHandling.h in Core.h as to not break code that include Core.h and use thing defined in ErrorHandling.h

deadalnix updated this revision to Diff 42905.Dec 15 2015, 1:52 PM

Fix go binding

Needs release note documentation and changes.

-eric

pcc added a subscriber: pcc.Dec 15 2015, 1:59 PM
pcc added inline comments.
bindings/go/llvm/analysis.go
17 ↗(On Diff #42905)

Please keep these lines sorted. The #include "llvm-c/Analysis.h" line in particular should come first here as the compiler will error on the first missing LLVM header.

deadalnix updated this revision to Diff 42910.Dec 15 2015, 2:16 PM

Add release note.
Sort include in the go binding by alphabetic order.

Some wordsmithing and a request for more text in the release notes.

Thanks!

docs/ReleaseNotes.rst
44–47 ↗(On Diff #42910)

How about:

"With this release, the C API headers have been reorganized to improve build time. <insert comment about what moved and where>. Nothing should change for projects including the headers directly, but may affect code that included headers transitively."

deadalnix updated this revision to Diff 42958.Dec 15 2015, 7:28 PM

Rewording

emaste added a subscriber: emaste.Dec 15 2015, 8:03 PM

Few more inline comments.

Thanks!

-eric

docs/ReleaseNotes.rst
45 ↗(On Diff #42959)

"Type specific defines moved to..."

Also you added an ErrorHandling file and there's no text for it.

include/llvm-c/ErrorHandling.h
2–11

Copy and paste problem.

include/llvm-c/Types.h
2–11

Cut and paste problem.

deadalnix updated this revision to Diff 43083.Dec 16 2015, 4:08 PM

Comments.

Some more text for you to fix :)

Thanks!

-eric

docs/ReleaseNotes.rst
53 ↗(On Diff #43083)

"Type specific defines have been moved to Type.h, and error handling routines have been moved to ErrorHandling.h. Both are included in Core.h so nothing should change for projects directly including the headers, but transitive dependencies may be affected."

include/llvm-c/ErrorHandling.h
2

Still have a copy and paste problem.

include/llvm-c/Types.h
2

Still have a copy and paste problem.

deadalnix added inline comments.Dec 16 2015, 4:29 PM
docs/ReleaseNotes.rst
53 ↗(On Diff #43083)

There are typedefs and opaque structs, not defines.

include/llvm-c/ErrorHandling.h
2

notsmartman

deadalnix updated this revision to Diff 43087.Dec 16 2015, 4:35 PM

Blurb reworded.
Header comments fixes.

echristo accepted this revision.Dec 16 2015, 4:53 PM
echristo edited edge metadata.

OK. LGTM.

Thanks!

-eric

This revision is now accepted and ready to land.Dec 16 2015, 4:53 PM
deadalnix updated this revision to Diff 43200.Dec 17 2015, 5:03 PM
deadalnix edited edge metadata.

Rebase and fix conflict.

Also can someone merge this ?

Done as of:

commit cca8dbee4ea6ee37ef32e1794008d4d15bbd566b
Author: Eric Christopher <echristo@gmail.com>
Date: Fri Dec 18 01:46:52 2015 +0000

Reorganize the C API headers to improve build times.

Type specific declarations have been moved to Type.h and error handling
routines have been moved to ErrorHandling.h. Both are included in Core.h
so nothing should change for projects directly including the headers,
but transitive dependencies may be affected.

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@255965

91177308-0d34-0410-b5e6-96231b3b80d8

mehdi_amini closed this revision.Jan 4 2016, 1:35 PM