This is an archive of the discontinued LLVM Phabricator instance.

[mlir][vector] let transfer_read and transfer_write take non-zero addrspace.
ClosedPublic

Authored by whchung on Apr 28 2020, 10:04 AM.

Details

Summary

Enhance lowering logic and tests so vector.transfer_read and
vector.transfer_write take memrefs on non-zero addrspaces.

Diff Detail

Event Timeline

whchung created this revision.Apr 28 2020, 10:04 AM
Herald added a project: Restricted Project. · View Herald Transcript
whchung added a project: Restricted Project.Apr 28 2020, 10:05 AM
whchung updated this revision to Diff 260725.Apr 28 2020, 12:17 PM

Rebase to tip.

aartbik added inline comments.Apr 28 2020, 2:34 PM
mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVM.cpp
837

This step 1. can use a bit more documentation now that it has become more eleborate

whchung updated this revision to Diff 260778.Apr 28 2020, 2:55 PM

Revise comment.

whchung marked 2 inline comments as done.Apr 28 2020, 2:56 PM
whchung added inline comments.
mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVM.cpp
837

@aartbik just added more comments.

ftynse accepted this revision.Apr 29 2020, 12:53 AM
ftynse added inline comments.
mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVM.cpp
844

Please drop trivial braces

This revision is now accepted and ready to land.Apr 29 2020, 12:53 AM
whchung updated this revision to Diff 260917.Apr 29 2020, 7:53 AM
whchung marked an inline comment as done.

Address review comment.

whchung marked an inline comment as done.Apr 29 2020, 7:53 AM
This revision was automatically updated to reflect the committed changes.
awpr added a subscriber: awpr.EditedFeb 9 2021, 3:12 PM

Is it important for the memory accesses here to be done via addrspace 0, or is that just an artifact of making the pointer type match up? It seems that for many architectures with multiple memory spaces, it won't be possible to address all memory via addrspace 0, and memory accesses should be done in the addrspace their memref lives in (as in https://reviews.llvm.org/D96380); but I'm not sure whether making that change would affect the use case this was meant for.