Few questions below. Please bear with me as I try to grok the proposal and the long stream of comments...

Add test for signext case

Looks good to me

Looks reasonable to me.

Looks good to me!

Looks good to me!

Address review comments

Refactor opcode selection into utility function and add test case

Just putting this up for Tobias to try... test case incoming.

Looks good! Thanks!

That's for... hmm... I don't know. I don't see how that block would be executed. The call lowering code has always been a mess, unfortunately.

Thanks for the comments. I'll try to get a new version of this up soon. As for http://reviews.llvm.org/D17872, it seems unlikely to be related. This bug is very specific to the selection of LDG. Though it may be generally related in so far as both are due to i8 handling. I really wish we had a better way of handling that.

This is an issue with LDG handling. canLowerToLDG() is returning true for an i8 load zero-extended to i32, but SelectLDGLDU cannot handle this case.

Cool! Glad this was fixed. Thanks!

At the hardware level, there is no 'param' address space. It's a PTX abstraction to handle parameter passing. There may be memory associated with it for complex calls, but it should never alias another address space. There's really no such thing as a pointer to a param space pointer.

Sorry, I know I've been slow to respond recently...

I'm not sure I understand the relation to non-param loads/stores. Param loads/stores will never alias another address space, so arbitrary reordering with other loads/stores should be legal. Can you show an example of what you're seeing?

Looks reasonable to me. Thanks!

Sorry, missed the original notifications.

What is the motivation for this? Is it reformatted with a tool?

Looks reasonable to me.

We do not have any tests for debug support, since it was never properly implemented. We have plenty of tests for other parts of the NVPTX AsmPrinter. Or are you referring to something else?

The documentation is actually correct here. PTXLdStInstCode is an internal enumeration that is independent of the public address space mapping. It's unfortunate that there is disagreement between the two mappings, but this should be fixed by changing the PTXLdStInstCode enum. See the AddressSpace enum in lib/Target/NVPTX/MCTargetDesc/NVPTXBaseInfo.h.

LGTM. Thanks!

LGTM! Thanks for fixing this; I just saw the PR go by. :)

LGTM

In D11622#216520, @eliben wrote:

In D11622#216514, @jholewinski wrote:

This looks reasonable for now. Longer term, I'd like to experiment with doing away with the typed registers completely and just use .bXX for all registers. Typing is nice for readability, but code gen can be cleaner without it. They all compile down to the same set of registers at the sass level, and I'd like to match the hardware as much as possible (given that we can't just generate sass).

It's a shame we can't generate SASS ;-) Working around limitations in ptxas makes LLVM's job somewhat miserable here.

This looks reasonable for now. Longer term, I'd like to experiment with doing away with the typed registers completely and just use .bXX for all registers. Typing is nice for readability, but code gen can be cleaner without it. They all compile down to the same set of registers at the sass level, and I'd like to match the hardware as much as possible (given that we can't just generate sass).

Looks reasonable to me, with the minor comment.

Looks reasonable to me.

Fair enough.

The algorithm looks good. Thanks for working on this!

The short answer is that ptxas doesn't handle vector registers very well. It may be good to revisit this, but ptxas currently prefers scalar ops.

LGTM

Looks reasonable to me

LTGM

Sounds reasonable to me.

Good catch; that was a legacy workaround that I missed.

What is the future expectation for NVPTXPeephole? Are you planning on adding additional transforms? If not, perhaps a more specific name is warranted. Otherwise, LGTM! Thanks!

Other than the issues raised by Jingyue, this looks good to me! Thanks!

Looks reasonable to me. Thanks!

Looks reasonable to me.

What is the purpose to using a static relocation model for NVPTX? I would prefer to just enforce a default here, especially since this is essentially unused for NVPTX.

Sorry for the delay. I was out of town when this was posted and completely missed it.

LGTM

LGTM

This looks good to me now. I agree that we should merge these two passes to prevent any ordering issues.

Change itself looks good, but these intrinsics are deprecated in favor of addrspacecast. A note to that effect should be added.

Comments inlined.

I committed a fix for this in r236000

LGTM

Sure, go for it! It may be a few days before I get around to looking at this.

Handling this aspect of PTX has always been a PITA. Internally, we customize lowerConstant to handle this case and create an MCExpr subclass (NVPTXGenericMCSymbolRefExpr) that handles the printing of "generic(<symbolref>)". Something like this will be needed upstream. Our internal version is a bit of a hack, so I'll want to come up with something cleaner for upstream. If you don't have time to tackle that, I'll try to put something together soon.

Looks good to me! Thanks for working on this!

LGTM. Thanks!

LGTM! Thanks for working on this!

Comments inline.

Looks good to me! Sorry for the delay, missed the original email notification.

NVPTX changes look reasonable to me.

LGTM.

This looks fine for now, considering the same approach is taken for the .globl directive. That said, we really need a better way to handle this. Ptxas supports just enough GNU-style syntax to let us get by without a custom AsmStreamer in most cases, but not enough to avoid these annoying issues.

This looks like a good start to me, but I'm not an expert on IndVarSimplify.

LGTM. Thanks!

LGTM. Thanks!

LGTM.

No issues internally, please commit. Thanks!

LGTM, but let me check with some folks internally.

This LGTM. Thanks for implementing this!

Closed by commit rL213253 (authored by @jholewinski).

Remove erroneous merge

Add test case