[mlir][LLVM] Replace readnone with memory effects
ClosedPublic
Actions

Authored by Dinistro on Jan 18 2023, 6:18 AM.

Details

Reviewers

gysit
ftynse
nicolasvasilache
dcaballe

Commits

rG9b9cfe77a50a: [mlir][LLVM] Replace readnone with memory effects

Summary

This commit introduces LLVM's MemoryEffects attribute and replaces the
deprecated usage of llvm.readnone in the LLVM dialect.
The absence of the attribute on a LLVMFuncOp implies that it might
access all kinds of memory. This semantic corresponds to llvm::Function's
behaviour.

Depends on D142002

Diff Detail

Repository: rG LLVM Github Monorepo

Event Timeline

Dinistro created this revision.Jan 18 2023, 6:18 AM

Herald added a reviewer: ftynse. · View Herald TranscriptJan 18 2023, 6:18 AM

Herald added a project: Restricted Project. · View Herald Transcript

Herald added subscribers: Moerafaat, zero9178, bzcheeseman and 22 others. · View Herald Transcript

Dinistro requested review of this revision.Jan 18 2023, 6:18 AM

Herald added a reviewer: nicolasvasilache. · View Herald TranscriptJan 18 2023, 6:18 AM

Herald added a reviewer: dcaballe. · View Herald Transcript

Herald added a project: Restricted Project. · View Herald Transcript

Herald added subscribers: stephenneuendorffer, nicolasvasilache. · View Herald Transcript

Harbormaster completed remote builds in B208471: Diff 490129.Jan 18 2023, 6:41 AM

gysit added inline comments.Jan 18 2023, 7:01 AM

mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td
386	nit: I think most other attributes use camelCase for LLVM attribute names infavor of using them as is.
mlir/lib/Conversion/FuncToLLVM/FuncToLLVM.cpp
390	nit: I would compute the attribute here and then use setMemoryAttr on the newFuncOp further below. That way we do not use the string to identify the right attribute.
mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
914	It does not seem like the function ever return failure?
mlir/test/Target/LLVMIR/function-attributes.mlir
1–18 ↗	(On Diff #490129)	is this file accidentally here? Generally it may be a good idea to factor out a separate test for the function attributes? However this seems to be a copy?