This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Add readonly and nocapture arg attributes to LLVM dialect
AcceptedPublic

Authored by ezhulenev on Nov 16 2022, 10:28 AM.

Diff Detail

Event Timeline

ezhulenev created this revision.Nov 16 2022, 10:28 AM
Herald added a project: Restricted Project. · View Herald Transcript
ezhulenev requested review of this revision.Nov 16 2022, 10:28 AM
ftynse accepted this revision.Nov 16 2022, 12:48 PM

Please do the verification in the dialect rather than in the translation.

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
972–987

I see there is code around doing the same, but this should really be checked in the dialect's verifier for argument attributes rather than here. Some of the code around largely predates that kind of verifier.

This revision is now accepted and ready to land.Nov 16 2022, 12:48 PM
ezhulenev added inline comments.Nov 16 2022, 1:34 PM
mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
972–987

Should it be legal to attach these attributes to non-llvm and non-pointer types if they'll become pointers once everything will be converted to LLVM? E.g. to memref (assuming it's going to use bare ptr conversion)? Or dialect verification should be identical to this translation?

vzakhari added inline comments.
mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
972–987

To the same note, I tried using isCompatibleType in https://reviews.llvm.org/D137049#change-zc5yndDKCEkj to narrow the verification for result attributes only to the compatible types. But it is subtle, e.g. verification may allow attributes for a type that will convert to LLVM type which does not allow an attribute. So I guess we may need to run types verification during translation to LLVM IR as well as part of LLVM dialect verification. Or we may drop the former and rely on LLVM verification, which will report an issue for the LLVM instruction that we try to create in a wrong way.

vzakhari added inline comments.Nov 17 2022, 11:06 PM
mlir/lib/Conversion/FuncToLLVM/FuncToLLVM.cpp
350

This should not be needed for unit attributes.