FWIW I've just hit the same issue, and this patch works great for me.

This change has caused Clang to start emitting erroneous warnings when using OpenCL's vector specifier, e.g. "%v4f", which doesn't seem to be covered by the format string analysis.

Thanks for the review! Would you be able to commit this on my behalf?

Ping - maybe this got lost in the noise.

Thanks - could you commit this on my behalf please?

This patch is now obsolete - the core issue has now been fixed by D27283. While that fix is a little more conservative (i.e. it doesn't replace the alloca when it sometimes could), it's good enough to fix all of the issues that I was experiencing in my downstream projects, so I'm happy to leave it at that.

Thanks for the speedy review!

Ping!

• Remove accidental debug statement

Thanks for the feedback, responses inline.

• Address review comments from mehdi_amini

In D28300#656855, @hfinkel wrote:

Is this optimization safe in general if the address spaces don't match? Could, for example, some of the users being affected be some target-specific intrinsic that is only valid for certain address spaces?

• Handle different global and function arg address spaces properly
• Use llvm_unreachable instead of assert(false)
• Rebase to current trunk

I don't know if it's too late, but I'd love for this to make it into the 4.0 release. Although this bug has been present for a while so perhaps isn't strictly a regression, there is a regression in downstream projects that I'm working on due to (valid) changes in the IR generated by Clang since 3.9 which expose this bug.

• Address review comments from arsenm

Fix memtransfer function type deduction

In D24580#543785, @jhen wrote:

The hack is in now. Please let me know if it works for you. Thanks!

In D24580#543354, @jhen wrote:

What do you think of the idea of doing a check in Python to see if the library base name is *.framework and then emitting the flag -lframework * instead of the raw path, if so? It's hacky, but it seems like it might work pretty much all the time.

In D24580#543239, @jhen wrote:

Thanks for bringing this up! I added logic to streamexecutor-config to add in the CUDA library for the --libs output if CUDA is enabled. It seems to work on Ubuntu. I'd definitely be glad to know if it works on OS X as well.

Does this also need the CUDA library to be added to streamexecutor-config output for --libs?

I believe LLVMConfig.cmake defines an LLVM_ENABLE_RTTI variable which can give you this information directly, without the need for the execute_process().

I've just noticed that this patch split the Utils target into a separate installed library (libutils.a), which breaks things building against SE using streamexecutor-config.

BTW, I don't have commit access, so I'm afraid someone else will have to commit this on my behalf.

Maybe I'm doing something wrong, but the single-quotes wrapping the include/lib paths given by --cppflags and --ldflags are not working for me. The compiler fails to find the StreamExecutor headers and libraries, and warns that the library directory doesn't exist. It's almost as if the quotes are being taken as part of the paths.

Looks good, with one minor nit.

Looks good to me.

That all looks nice.

Hmm, maybe a false alarm.

The getOrDie function seems to be generating an additional 'unchecked error' message when dying. For example, this trivial program (which should obviously generate an error):

Aha, this explains why I couldn't find these classes in the docs yesterday.

Not sure what the protocol is for finding issues in patches that have already been committed - should I be posting to parallel_libs-dev instead?

Just some mild pedantry from me.

General question: if we have variants of these memcpy methods that take ElementCount parameters to allow for partial copies to/from the device allocations, should we also have variants with an Offset parameter as well to allow for partial copies that don't start at the origin?

One last nit from me.

FWIW, this design feels a lot cleaner to me :-)

I think I understand the motivation for restricting the use of StreamExecutor::launch/memcpy etc to only the Stream class via the StreamKey struct, but I'm not sure I see the need for the StreamExecutorKey struct. It seems like this is currently just protecting the Stream constructor, but is it really a problem if someone can write new Stream(Executor, nullptr) as well as Executor->createStream()? Don't have strong opinions about this, just curious.

In D18369#387560, @yaxunl wrote:

In D18369#385799, @Anastasia wrote:

Is there any chance we could factor out the common bits into separate files to avoid large code repetition? I would imagine it should be quite doable as libs of each standard contain incremental changes.

I saw some inconsistencies in the common part of the 1.2 and 2.0 headers. I will try to consolidate them first then try to split.