This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Add alloca address space handling to the data layout subsystem
ClosedPublic

Authored by jsjodin on Feb 23 2023, 10:01 AM.

Details

Summary

This patch adds alloca address space information to the data layout interface
and implementation in the DLTI dialect. This is needed for targets that use
separate address spaces for local/stack data.

Diff Detail

Event Timeline

jsjodin created this revision.Feb 23 2023, 10:01 AM
jsjodin requested review of this revision.Feb 23 2023, 10:01 AM
krzysz00 requested changes to this revision.Feb 23 2023, 10:45 AM

Since we want to be able to use this whenever someone's creating a memref::AllocaOp, we want a generic Attribute-valued memory space key.

However, we could also have both the unsigned-valued allocaAddressSpace and the Attribute-valued allocaMemorySpace as separate keys, with addressSpace used in LLVM lowering on down and allocaMemorySpace used in MLIR proper. Though that'd require some documentation to make sure people understand which is which ...

mlir/include/mlir/Dialect/DLTI/DLTI.h
102
  1. For consistency with memref, can we call this getAllocaMemorySpaceIdentifier()
mlir/include/mlir/Interfaces/DataLayoutInterfaces.h
61

I'd argue this should also be Attribute, and that, when dealing with the *ToLLVMs, it should be sent through the attribute conversion process.

192

mutable Attribute memorySpace

mlir/lib/Dialect/DLTI/DLTI.cpp
340

Strong disagree with the restriction to integer attributes, consider that this needs to work for #gpu.address_space<Private>

This revision now requires changes to proceed.Feb 23 2023, 10:45 AM

... in further comments, if we want to go with the address space and memory space as distinct data layout entries system, I'd be willing to review this patch on its own and leave the memory space part to another patch.

... in further comments, if we want to go with the address space and memory space as distinct data layout entries system, I'd be willing to review this patch on its own and leave the memory space part to another patch.

It would probably be better to have a generic entry system that can work for anything

mlir/include/mlir/Interfaces/DataLayoutInterfaces.h
61

Yes, that is more general. I will work on an updated patch.

192

Yes this makes sense. Each dialect (like LLVMIR) that uses the data layout could have their own convenience functions to handle the values. Where would the best way to put these functions?

Also, how would the default case be handled (currently returns 0)?

Re the default value of getAllocaMemorySpcae(), that would be the null attribute, aka {} aka nullptr, which is the current default.

jsjodin updated this revision to Diff 500859.Feb 27 2023, 11:37 AM

Use Attribute instead of integers to encode memory spaces. Change address space -> memory space.

jsjodin marked 4 inline comments as done.Feb 27 2023, 11:38 AM
krzysz00 accepted this revision.Feb 28 2023, 9:22 AM

Looks good to me - though I'm not sure if we want @ftynse or someone else more familiar with DTLI to take a look before landing.

This revision is now accepted and ready to land.Feb 28 2023, 9:22 AM

Looks good to me - though I'm not sure if we want @ftynse or someone else more familiar with DTLI to take a look before landing.

Yes, I will let @ftynse approve.

ftynse accepted this revision.Mar 20 2023, 9:37 AM
ftynse added inline comments.
mlir/lib/Target/LLVMIR/ModuleImport.cpp
236

Ultra-nit: terminate comments with a dot.

mlir/unittests/Interfaces/DataLayoutInterfacesTest.cpp
278

Can we have a test with non-null memory space?

Sorry for delay, already-approved patches don't show up in the review stream :(

Sorry for delay, already-approved patches don't show up in the review stream :(

No problem, I suspected that might be the case.

mlir/lib/Target/LLVMIR/ModuleImport.cpp
236

Ultra-nit: terminate comments with a dot.

Will fix.

mlir/unittests/Interfaces/DataLayoutInterfacesTest.cpp
278

Sure, I will work on another test case.

jsjodin updated this revision to Diff 506717.Mar 20 2023, 2:14 PM

Added unittest with non-null memory space. Fixed comments to end with '.'.