This is an archive of the discontinued LLVM Phabricator instance.

[NVPTX] Annotate param loads/stores as mayLoad/mayStore.
ClosedPublic

Authored by jlebar on Feb 19 2016, 6:55 PM.

Details

Summary

Tablegen was unable to determine that param loads/stores were actually
reading or writing from memory. I think this isn't a problem in
practice for param stores, because those occur in a block right before
we make our call. But param loads don't have to at the very beginning
of a function, so should be annotated as mayLoad so we don't incorrectly
optimize them.

Diff Detail

Event Timeline

jlebar updated this revision to Diff 48567.Feb 19 2016, 6:55 PM
jlebar retitled this revision from to [NVPTX] Annotate param loads/stores as mayLoad/mayStore..
jlebar updated this object.
jlebar added a reviewer: jholewinski.
jlebar added a subscriber: llvm-commits.
jlebar added a subscriber: tra.Feb 19 2016, 6:56 PM

Friendly ping. We're looking at a correctness issue which appears that it might be related to this; would be nice to have this resolved.

jholewinski accepted this revision.Feb 29 2016, 5:53 PM
jholewinski edited edge metadata.

Sorry, missed the original notifications.

The change looks reasonable. Do you have a particular example that shows the issue you're addressing here? The pre-call stores and post-call loads should be properly chained to prevent invalid reordering.

This revision is now accepted and ready to land.Feb 29 2016, 5:53 PM

Sorry, missed the original notifications.

Do you have a particular example that shows the issue you're addressing here? The pre-call stores and post-call loads should be properly chained to prevent invalid reordering.

We're seeing some (non-param) loads suspiciously being sunk. This may be unrelated, but it's helpful to rule out...

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?

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?

I looked more closely at the thing we were looking at, and you're right, it's unrelated.

However, param loads/stores *can* alias with the global address space, yes? So given something like

a = ld.global [A]
st.param [B]
if (...) {
  // use a
}

we cannot safely sink the load of 'a' into the branch:

st.param [B]
if (...) {
  a = ld.global [A]
  // use a
}

because A and B may alias.

Or, even if that's not possible, then one could conceive of a situation where we somehow have a param pointer aliasing another param:

param i32 A;
param i32* B = &A;
// now we have the same problem.

Anyway I agree this is mostly academic, given the kind of code we (currently) generate.

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.

Anyway, I have no issues with this change. These are technically loads/stores. Thanks!

This revision was automatically updated to reflect the committed changes.