Page MenuHomePhabricator

[GlobalISel] Add the proper cmake plumbing.
ClosedPublic

Authored by qcolombet on Jan 7 2016, 4:59 PM.

Details

Summary

Hi,

This patch adds the necessary plumbing to cmake to build the sources related to GlobalISel.
To be able to test this plumbing, this patch also adds an empty implementation of the IRTranslator.
Do not waste time on this empty implementation, the API will likely evolve as the prototype goes.

After this patch lands, it will be possible to build sources related to GlobalISel by adding -DBUILD_GLOBAL_ISEL=ON. By default, this is OFF, thus GlobalISel sources will not impact people that do not explicitly opt-in.

OK to commit?

Thanks,
-Quentin

Diff Detail

Repository
rL LLVM

Event Timeline

qcolombet updated this revision to Diff 44291.Jan 7 2016, 4:59 PM
qcolombet retitled this revision from to [GlobalISel] Add the proper cmake plumbing..
qcolombet updated this object.
qcolombet added reviewers: chandlerc, echristo, hfinkel.
qcolombet set the repository for this revision to rL LLVM.
qcolombet added a subscriber: llvm-commits.

Hi Quentin,

There should be a line in the top-level CMakeLists.txt exposing BUILD_GLOBAL_ISEL to the cache. Otherwise, if you re-cmake, you won't build global isel the second time!

Something like

set(BUILD_GLOBAL_ISEL 0 CACHE STRING "Experimental: Build GlobalISel")

I'd also prefer this to have a "LLVM_" prefix as all other global cache variables do in our CMakeLists.

Cheers,

James

qcolombet updated this revision to Diff 44336.Jan 8 2016, 10:22 AM
  • Add LLVM prefix to the variable name
  • Set the variable into the cache

Hi James,

Thanks for the feedbacks.
I’ve updated the patch accordingly.

Cheers,
-Quentin

MatzeB added a subscriber: MatzeB.Jan 11 2016, 11:25 AM

The commit reads somewhat strange as it looks like a TODO file split into comments. I guess that is fine and too early to review until we have some real implementations. The cmake parts look good to me now. Nitpicks below.

include/llvm/CodeGen/GlobalISel/IRTranslator.h
25 ↗(On Diff #44336)

Isn't this obvious?

31–38 ↗(On Diff #44336)

Before the class I would expect a doxygen (///) comment describing what the pass does.
I think high-level concerns and discussions should rather go into the \file section of the .cpp file.

The same applies to most of the following comments, they are not doxygen and often describe a lot of high-level concerns instead of just describing what the function does...

44 ↗(On Diff #44336)

grammar.

65 ↗(On Diff #44336)

grammar. Could use doxygen style // .. // when describing a larger section of the file.

lib/CodeGen/GlobalISel/LLVMBuild.txt
18–22 ↗(On Diff #44336)

Does this give us a way to disable/enable GlobalISel in the Makefiles? If not maybe just leave out of the Makefile system for now as there is a good chance that the Makefiles are removed before GlobalISel is relevant for most people.

Hi Matthias,

The commit reads somewhat strange as it looks like a TODO file split into comments.

This is really what this is :). I could it I guess to have just the cmake part, but we wouldn’t have anything to test the cmake then.

I didn’t quite get your comment on the "Makefile part”. I am admittedly not familiar with the internals of the build system. See my inlined comments.

Thanks,
-Quentin

include/llvm/CodeGen/GlobalISel/IRTranslator.h
25 ↗(On Diff #44336)

I don’t know how common it is to use that kind of comments, but I personally like to call it out so that it is obvious that this section is for forward declarations and nothing else.
I can take it out if you prefer.

31–38 ↗(On Diff #44336)

Yeah, at this point, it is more todos than actual doxygen comments.

lib/CodeGen/GlobalISel/LLVMBuild.txt
18–22 ↗(On Diff #44336)

What do you mean “in the Makefiles”?

If you do add the dependency on GlobalISel, the libLLVMGlobalISel should be built, right?

Moreover, I thought those LLVMBuild.txt files were mandatory.

MatzeB added inline comments.Jan 11 2016, 1:37 PM
include/llvm/CodeGen/GlobalISel/IRTranslator.h
25 ↗(On Diff #44336)

It's probably a personal thing but I often think a reduced amount of information is better than large amounts of obvious/unnecessary information as there is always a (really really tiny in this case) cost of reading/processing it. Sure this particular example is probably not even worth the time thinking or discussing it in a review. So no objection from me one way or the other.

lib/CodeGen/GlobalISel/LLVMBuild.txt
18–22 ↗(On Diff #44336)

Hmm I assumed the LLVMBuild.txt is only used for the Makefile build, because when I added some simple libraries/executables in my personal experiments everything worked fine without them and the information looked like a duplication of everything in CMakeLists.txt. But you are right I just checked again and the cmake build is doing something with the information as well, so they are apparently necessary for cmake and Makefile builds.

qcolombet updated this revision to Diff 44576.Jan 11 2016, 3:54 PM
  • Grammar.
  • Add a comment stating that the comments need proper doxygen style.
qcolombet updated this revision to Diff 44581.Jan 11 2016, 4:48 PM

Typo in comment.

Hi Matthias,

Is it okay to commit?

Cheers,
-Quentin

MatzeB accepted this revision.Jan 19 2016, 10:49 AM
MatzeB added a reviewer: MatzeB.

Hi Matthias,

Is it okay to commit?

sure.

This revision is now accepted and ready to land.Jan 19 2016, 10:49 AM
echristo accepted this revision.Jan 19 2016, 10:54 AM
echristo edited edge metadata.

I'm going to giggle a bit at the use of Toolkit, but LGTM as well.

Thanks!

-eric

This revision was automatically updated to reflect the committed changes.

It seems lldb xcode build started to fail after this CL - http://lab.llvm.org:8011/builders/lldb-x86_64-darwin-13.4/builds/8218

llvm-config: error: missing: /Users/lldb_build/lldbSlave/buildDir/lldb/llvm-build/Release+Asserts/x86_64/Release+Asserts/lib/libLLVMGlobalISel.a
/Users/lldb_build/lldbSlave/buildDir/lldb/llvm/Makefile.rules:1080: *** llvm-config --libs failed. Stop.

Can you provide Makefile configuration for this library?

I believe r258379 should fix the problem.

Let me know if this is not the case.

Thanks,
-Quentin