This is an archive of the discontinued LLVM Phabricator instance.

[NVPTX] Handle ldg created from sign-/zero-extended load
ClosedPublic

Authored by jholewinski on Mar 10 2016, 10:55 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

jholewinski retitled this revision from to [NVPTX] Handle ldg created from sign-/zero-extended load.
jholewinski updated this object.
jholewinski added a reviewer: jingyue.

+jlebar@

I am OOO and maybe unable to review this until next week.

jlebar edited edge metadata.Mar 11 2016, 1:52 PM

I'm happy to review this, but I need to understand the surrounding code better first, so it may take me a day or two. These two comments are as far as I got before I realized that. :)

lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp
1326 ↗(On Diff #50310)

Do you think this warrants a comment?

In particular I'm confused why we do this unconditionally, even for non-vector loads.

test/CodeGen/NVPTX/bug26185.ll
1 ↗(On Diff #50310)

It might be helpful to have a comment here explaining what we're checking. The bug has some explanation, but I think it's not entirely helpful to someone approaching this file without any context.

jlebar added a comment.EditedMar 11 2016, 4:21 PM

(I also wonder if this is related to http://reviews.llvm.org/D17872 .)

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.

lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp
1326 ↗(On Diff #50310)

Which part? The setting of NodeVT, or the following loop? The whole point is just to create the result type for the load instruction. For scalars, this will be (iN, Other); for vectors, it will be (iN, ..., Other). Do you have a suggestion for a cleaner way of writing this?

I agree that a comment is warranted.

test/CodeGen/NVPTX/bug26185.ll
1 ↗(On Diff #50310)

Sure; sounds good.

jlebar accepted this revision.Mar 14 2016, 6:42 PM
jlebar edited edge metadata.

Okay, I understand this! Seems...I don't want to say "sane". "Called for"?

lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp
1326 ↗(On Diff #50310)

Yeah, the mechanics of the vector we're building up here are clear to me. But it's not clear *why* we always convert i8s to i16s in said vector. See below, though, I think this is an ISA detail I was missing.

2057 ↗(On Diff #50310)

Maybe it would be worth explaining somewhere that the whole reason we're doing this is because there are no i8 registers? That is probably obvious to you, but certainly wasn't to me! Once I got that, this all started to make sense -- we load into an i16, but the top bits are undef, so we clear them and then do the zero extension.

2110 ↗(On Diff #50310)

Not sure what this is supposed to stand for, and "Opr" doesn't appear anywhere else in llvm.

This revision is now accepted and ready to land.Mar 14 2016, 6:42 PM

Should we land this? It will fix PR26185.

This revision was automatically updated to reflect the committed changes.