Congrats on your first patch!

I'll land this for you.

• What's different in this patch vs the previous one?
• *Disabled a hip test on Windows that's breaking on head.* Can you clarify: Is this test broken at HEAD, or does it break with your patch?

In D119207#3327476, @shangwuyao wrote:

Thanks for the review, if it looks good, can we get this to land now? Otherwise more comments are welcome!

[CUDA][SPIRV] Convert CUDA kernels to SPIR-V kernels

Pushed for Daniele:

I defer to Art.

Let me summon them.

It appears that mayLoad was omitted for a reason, but I cannot figure out why. If a load is done from a read-only memory, it should still be treated as a load, right?

Presumably as a separate commit we should add tests to the test_suite repository to ensure that this at least still compiles with different versions of CUDA?

Okay, I give up on the phab interface. It's unreadable with all the existing
comments and lint errors.

Depending on which particular operation is used, the arguments vary, too.

One alternative would be to use run-time dispatch, but, given that texture lookup is a single instruction, the overhead would be substantial-to-prohibitive.

Is it a pain to add a test? I know the reproducer from the bug still crashes llvm, not sure if it's hard to reproduce the i128 issue independently.

Thanks, Art.

... let's try adding some other reviewers who've had their fingers in SemaOverload ...

Thank you for the review, Richard!

@rsmith I have no idea if the other test change is correct, and gnu cxxfilt doesn't work with that mangled name. Hope this doesn't actually break something...

Other than the declval issue discussed above, this looks reasonable to me. I don't notice anything wrong with the standard library reimplementations here.

I guess you need me or Michael to push this. Happy to do so once you're
happy with it.

I can't argue with this.

Looks great to me. Thanks, Art.

...in which we all remember why we like writing this recursively. :D

This is going to be so great. Thanks, Art.

Maybe add a comment on assertfail that we don't want it to be noreturn because blah blah? So someone doesn't come back and add it when we're not looking. :)

Thank you for the review!

I guess I was confused by the function name

Rename noteNumOverloadCandidatesShown -> overloadCandidatesShown.

Thank you for your comments, Aaron!

Not sure who can review this, but looking through blame it seems like maybe @aaronpuchert?

In D92210#2559113, @maleadt wrote:

Bump, anything else needed here?

Hi, welcome! Thank you for the careful and well-motivated first commit. (I also see https://github.com/ccache/ccache/issues/772, hooray for noticing that...)

This looks reasonable to me (and I appreciate all the debugging in the Julia bug!), but I have never touched this code, so I don't 100% feel comfortable approving the change.

In case it's not used in PHI or SELECT and cannot ensure the result is also a pointer to the parameter space, we could skip alloca insertion.

I don't believe there's any exception to prove deduction [of the readonly attribute] wrong.

This looks really simple, which is awesome. I am enthusiastic. But I am worried it may not be correct.

How fun. :)

I am legit excited about this if we could figure out how to make it work, but I don't have anything to add beyond what tra said.

The bottomline seems to be that the proposed formalism achieves the following: <snip>

LGTM. I think the change would make sense for CUDA, too. @jlebar - WDYT?

Man, phab doesn't make this easy, does it?

Hi. :) A few people pinged me asking for my feedback here, since I touched the convergent attr way back in the day, for CUDA.

LGTM modulo emankov's comment.

Fix bug in test command line, and fix commit message.

It does not quite match what we do here.

Hey, I'm leaving on a vacation tomorrow and didn't have a chance to get to
this review today.

It looks like 11.1 doesn't have a version.txt file

It should. I did mention in a previous comment that > Looks like the const-ness check should not be there, either. I need to revise the patch.

OK, now I'm starting to I understand this change..

OK, backing up, what are the semantics of static on __constant__, __device__, and __shared__?

wha... As you know, const doesn't mean anything, that can be const-casted away. And then you'll be able to observe that this nominally-static variable is just a normal variable.

I know it comes in a separate change, but can we add a check to the test-suite?

LGTM, and can we write a test in the test-suite?

tra and I talked offline and I...think this makes sense.

@RKSimon and others, thank you for the review and comments. Sorry for my delay here; this has changed from being my day job to my weekend hobby, and that made a bigger difference in my responsiveness than I'd like or than I expected.

Update per comments.