Page MenuHomePhabricator

[NVPTX] Add a truncate DAG node to some calls.
ClosedPublic

Authored by jlebar on Mar 3 2016, 4:44 PM.

Details

Summary

Previously, we were running afoul of the assertion

EVT(CLI.Ins[i].VT) == InVals[i].getValueType() && "LowerCall emitted a value with the wrong type!"

in SelectionDAGBuilder.cpp when running the NVPTX/i8-param.ll test.
This is because our backend (for some reason) treats small return values
as i32, but it wasn't ever truncating the i32 back down to the expected
width in the DAG.

Unclear to me whether this fixes any actual bugs -- in this test, at
least, the generated code is unchanged.

Diff Detail

Repository
rL LLVM

Event Timeline

jlebar updated this revision to Diff 49785.Mar 3 2016, 4:44 PM
jlebar retitled this revision from to [NVPTX] Add a truncate DAG node to some calls..
jlebar updated this object.
jlebar added a reviewer: jingyue.
jlebar added subscribers: jholewinski, tra, llvm-commits.
jingyue added inline comments.Mar 3 2016, 5:25 PM
lib/Target/NVPTX/NVPTXISelLowering.cpp
1641 ↗(On Diff #49785)

Why shouldn't we set needTruncate here too?

jlebar added inline comments.Mar 4 2016, 9:52 AM
lib/Target/NVPTX/NVPTXISelLowering.cpp
1641 ↗(On Diff #49785)

Interesting... I think this comment is wrong? If returning i1 or i8 (or i16), we should hit the first branch. This branch should be for *non-int* types of size less than 16? But then, I'm not sure what that is, so I'm not sure when we ever run this code.

jingyue resigned from this revision.Mar 15 2016, 10:56 AM
jingyue edited reviewers, added: jholewinski; removed: jingyue.

I am not clear with this part either. jholewinski@, can you comment on this?

Justin, friendly ping.

jholewinski edited edge metadata.Mar 31 2016, 5:36 AM

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.

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.

Hey, a real, live Chesterton's Fence!

In the interest of not changing things that I don't understand, I think I'll leave out the needTruncate on this path, and add a comment saying we may need it, if we can figure out what the branch is for at all.

jlebar updated this revision to Diff 52304.Mar 31 2016, 3:46 PM
jlebar edited edge metadata.

Add comment about branch we don't understand.

jlebar removed a subscriber: jingyue.

I would like to check in the one fix we know makes sense so that I can reenable the assertion we're hitting. (Currently you have to pass -debug to hit it, which is why we missed it in the first place.)

jingyue accepted this revision.Mar 31 2016, 4:01 PM
jingyue edited edge metadata.

Other than that FIXME, pretty straightforward. LGTM

lib/Target/NVPTX/NVPTXISelLowering.cpp
1621 ↗(On Diff #52304)

Maybe call it BitWidth instead?

This revision is now accepted and ready to land.Mar 31 2016, 4:01 PM
jlebar marked an inline comment as done.Mar 31 2016, 6:06 PM
jlebar added inline comments.
lib/Target/NVPTX/NVPTXISelLowering.cpp
1621 ↗(On Diff #52304)

I'll fix that and the variable capitalization here in a separate change, which I'll land right after this one.

jlebar marked an inline comment as done.Mar 31 2016, 6:10 PM
jlebar added inline comments.
lib/Target/NVPTX/NVPTXISelLowering.cpp
1621 ↗(On Diff #52304)

Actually, I take it back. The whole file is actually pretty consistent with "sz" and lower-case "align". The LLVM style guide says to be consistent and to avoid big refactorings unless you're touching all the code anyway. I think we should probably leave this sleeping dragon un-tickled for now.

This revision was automatically updated to reflect the committed changes.