Page MenuHomePhabricator

[CUDA] Generate CUDA's printf alloca in its function's entry block.
ClosedPublic

Authored by jlebar on Jan 27 2016, 6:23 PM.

Details

Summary

This is necessary to prevent llvm from generating stacksave intrinsics
around this alloca. NVVM doesn't have a stack, and we don't handle said
intrinsics.

Diff Detail

Repository
rL LLVM

Event Timeline

jlebar updated this revision to Diff 46206.Jan 27 2016, 6:23 PM
jlebar retitled this revision from to [CUDA] Generate CUDA's printf alloca in its function's entry block..
jlebar updated this object.
jlebar added a reviewer: rnk.
jlebar added subscribers: tra, echristo, jhen, cfe-commits.
echristo added inline comments.Jan 27 2016, 6:41 PM
lib/CodeGen/CGCUDABuiltin.cpp
109 ↗(On Diff #46206)

Not quite, you'll want to use AllocaInsertPt for this or even CreateTempAlloca.

jlebar updated this revision to Diff 46293.Jan 28 2016, 10:25 AM

Address echristo's review comments.

jlebar updated this object.Jan 28 2016, 10:27 AM
jlebar marked an inline comment as done.
jlebar added inline comments.
lib/CodeGen/CGCUDABuiltin.cpp
109 ↗(On Diff #46206)

Aha. Used AllocaInsertPt because it doesn't seem that there's an overload of CreateTempAlloca that takes an explicit size.

jlebar marked an inline comment as done.Jan 28 2016, 10:27 AM
rnk added inline comments.Jan 28 2016, 11:50 AM
lib/CodeGen/CGCUDABuiltin.cpp
105–108 ↗(On Diff #46293)

The fact that allocas for local variables should always go in the entry block is pretty widespread cultural knowledge in LLVM and clang. Most readers aren't going to need this comment, unless you expect that people working on CUDA won't have that background. Plus, if you use CreateTempAlloca, there won't be any question about which insert point should be used.

109 ↗(On Diff #46293)

You can still use CreateTempAlloca by making an [i8 x N] LLVM type. You'll have to use CreateStructGEP below for forming GEPs. Overall I think that'd be nicer, since you don't need to worry about insertion at all.

echristo added inline comments.Jan 28 2016, 11:54 AM
lib/CodeGen/CGCUDABuiltin.cpp
109 ↗(On Diff #46293)

+1 :)

112 ↗(On Diff #46293)

Also you'd have wanted to insert it before anyhow.

jlebar updated this revision to Diff 46314.Jan 28 2016, 2:30 PM
jlebar marked an inline comment as done.

Use a struct rather than an i8 buffer.

jlebar marked 3 inline comments as done.Jan 28 2016, 2:32 PM

Thank you for the reviews.

Please have another look; I switched to using a struct proper. It's a lot cleaner! We're now assuming that the struct is aligned in the same way as vprintf wants, but if anything I expect this new code is more likely to match what it wants.

lib/CodeGen/CGCUDABuiltin.cpp
105–108 ↗(On Diff #46293)

OK, yeah, I also don't like comments that explain something that everyone other than the author knows. Thanks.

echristo accepted this revision.Jan 28 2016, 2:45 PM
echristo added a reviewer: echristo.

One inline nit then LGTM.

-eric

lib/CodeGen/CGCUDABuiltin.cpp
87 ↗(On Diff #46314)
  • on the wrong side ;)
This revision is now accepted and ready to land.Jan 28 2016, 2:45 PM
jlebar marked an inline comment as done.Jan 28 2016, 4:02 PM
jlebar added inline comments.
lib/CodeGen/CGCUDABuiltin.cpp
87 ↗(On Diff #46314)

Argh, I really need to set up a linter. I'm still doing readability reviews, and I cannot brain two styles. Sorry to keep wasting your time with silly stuff like this.

This revision was automatically updated to reflect the committed changes.
jlebar marked an inline comment as done.
echristo added inline comments.Jan 28 2016, 4:06 PM
lib/CodeGen/CGCUDABuiltin.cpp
87 ↗(On Diff #46314)

You could just use clang-format on everything :)

jlebar added a subscriber: jlebar.Jan 28 2016, 4:09 PM

Do you have a script that will take as input a commit range and git
commit --amend clang-tidy fixes for lines modified in that range?
Because if so,

a) I would be your best friend forever, and
b) It should be simple to convert that into a linter for arc to catch
the case when I forget to run said tool.

Hm, well, https://llvm.org/svn/llvm-project/cfe/trunk/tools/clang-format/git-clang-format
is close... Not sure if that triggers the bff clause, will consult my
attorney.