llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clangir Author: Adam Smith (adams381) <details> <summary>Changes</summary> side_effect was carrying three facts at once. It described the memory a callee may touch, and it was also the only thing lowering derived nounwind and willreturn from, so any callee with a known memory effect got both. That is wrong for one that can throw, and willreturn had no other representation in CIR at all. const and pure now record the three separately, and the enum is replaced by a structured #cir.memory_effects carrying the same six ModRef slots as the LLVM dialect. On an operation it prints compactly, as memory(none) or memory(read, argmem: readwrite). No lowered IR changes. Assisted-by: Cursor / claude-opus-5 --- Patch is 51.46 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/223890.diff 19 Files Affected: - (modified) clang/include/clang/CIR/Dialect/IR/CIRAttrs.td (+58-23) - (modified) clang/include/clang/CIR/Dialect/IR/CIRDialect.td (+4-1) - (modified) clang/include/clang/CIR/Dialect/IR/CIROps.td (+10-8) - (modified) clang/include/clang/CIR/Interfaces/CIROpInterfaces.td (+2-2) - (modified) clang/lib/CIR/CodeGen/CIRGenCall.cpp (+16-15) - (modified) clang/lib/CIR/CodeGen/CIRGenModule.cpp (+1-2) - (modified) clang/lib/CIR/CodeGen/CIRGenModule.h (+1-3) - (modified) clang/lib/CIR/Dialect/IR/CIRDialect.cpp (+147-41) - (modified) clang/lib/CIR/Dialect/Transforms/CIRTransformUtils.cpp (+2-7) - (modified) clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp (+53-72) - (modified) clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.h (-5) - (modified) clang/test/CIR/CodeGen/call.c (+2-2) - (modified) clang/test/CIR/CodeGen/side-effect.cpp (+4-4) - (modified) clang/test/CIR/IR/call.cir (+28-4) - (modified) clang/test/CIR/IR/enum-attrs.cir (+9-5) - (added) clang/test/CIR/IR/invalid-memory-effects.cir (+50) - (added) clang/test/CIR/Lowering/memory-effects.cir (+39) - (modified) clang/test/CIR/Transforms/flatten-preserve-attrs.cir (+28-10) - (modified) clang/test/CIR/Transforms/idiom-recognizer.cpp (+1-1) ``````````diff diff --git a/clang/include/clang/CIR/Dialect/IR/CIRAttrs.td b/clang/include/clang/CIR/Dialect/IR/CIRAttrs.td index f263cd30eb458..0e8d7177ee6b0 100644 --- a/clang/include/clang/CIR/Dialect/IR/CIRAttrs.td +++ b/clang/include/clang/CIR/Dialect/IR/CIRAttrs.td @@ -1953,39 +1953,74 @@ def CIR_BlockAddrDiffAttr } //===----------------------------------------------------------------------===// -// Side Effect +// Memory Effects //===----------------------------------------------------------------------===// -def CIR_SideEffect : CIR_I32Enum< - "SideEffect", "allowed side effects of a function", [ - I32EnumCase<"All", 0, "all">, - I32EnumCase<"Pure", 1, "pure">, - I32EnumCase<"Const", 2, "const"> +def CIR_ModRefInfo : CIR_I32Enum< + "ModRefInfo", "how a function accesses one class of memory", [ + I32EnumCase<"NoModRef", 0, "none">, + I32EnumCase<"Ref", 1, "read">, + I32EnumCase<"Mod", 2, "write">, + I32EnumCase<"ModRef", 3, "readwrite"> ]> { let description = [{ - The side effect attribute specifies the possible side effects of a function - or the target of a call operation. This is an enumeration attribute with - the following possible values: - - - all: The function or callee can have any side effects. This is the default - if no side effects are explicitly listed. - - pure: The function or callee may read data from memory, but it cannot - write data to memory. This has the same effect as the GNU C/C++ attribute - `__attribute__((pure))`. - - const: The function or callee may not read or write data from memory. This - has the same effect as the GNU C/C++ attribute `__attribute__((const))`. + Whether a function may read and/or write one class of memory. The + spellings match LLVM's, so `none` means neither read nor written and + `readwrite` means both may happen. + }]; +} - Examples: +def CIR_MemoryEffectsAttr : CIR_Attr<"MemoryEffects", "memory_effects"> { + let summary = "What memory a function or callee may touch"; + let description = [{ + The memory a function or the target of a call may read and write, given + per class of memory so that a promise about one class does not have to be + a promise about all of them. + + `other` covers every location not named by one of the remaining + parameters, so a function that only touches its pointer arguments has + `other = none` and `arg_mem = readwrite`. + + An absent attribute means the effects are unknown, which is the + conservative default and what a function with no memory annotation gets. + + On an operation the effects print as the access shared by every class not + named individually, followed by only the classes that differ from it. + The shared access is left out when it is `none` and some class does + differ, since an unnamed class reads back as `none`. ``` - %2 = cir.call @add(%0, %1) : (!s32i, !s32i) -> !s32i - %2 = cir.call @add(%0, %1) : (!s32i, !s32i) -> !s32i side_effect(pure) - %2 = cir.call @add(%0, %1) : (!s32i, !s32i) -> !s32i side_effect(const) + cir.func private @a() -> !s32i memory(none) + cir.func private @b() -> !s32i memory(read) + cir.func private @c(!cir.ptr<!s32i>) memory(argmem: readwrite) + cir.func private @d(!cir.ptr<!s32i>) memory(read, argmem: readwrite) + %2 = cir.call @a() memory(none) : () -> !s32i ``` }]; -} -def CIR_SideEffectAttr : CIR_EnumAttr<CIR_SideEffect, "side_effect">; + let parameters = (ins "ModRefInfo":$other, "ModRefInfo":$arg_mem, + "ModRefInfo":$inaccessible_mem, + "ModRefInfo":$errno_mem, "ModRefInfo":$target_mem0, + "ModRefInfo":$target_mem1); + + let builders = [ + AttrBuilder<(ins "ModRefInfo":$all), [{ + return $_get($_ctxt, all, all, all, all, all, all); + }]> + ]; + + let extraClassDeclaration = [{ + /// Whether every class of memory carries the same access as `other`. + bool isUniform() const { + return getArgMem() == getOther() && getInaccessibleMem() == getOther() && + getErrnoMem() == getOther() && getTargetMem0() == getOther() && + getTargetMem1() == getOther(); + } + }]; + + let assemblyFormat = "`<` struct(params) `>`"; + let canHaveIllegalCXXABIType = 0; +} //===----------------------------------------------------------------------===// // StaticLocalGuardAttr diff --git a/clang/include/clang/CIR/Dialect/IR/CIRDialect.td b/clang/include/clang/CIR/Dialect/IR/CIRDialect.td index e323eff0b9aa6..463d158c08f8b 100644 --- a/clang/include/clang/CIR/Dialect/IR/CIRDialect.td +++ b/clang/include/clang/CIR/Dialect/IR/CIRDialect.td @@ -46,7 +46,9 @@ def CIR_Dialect : Dialect { static llvm::StringRef getCalleeAttrName() { return "callee"; } static llvm::StringRef getNoThrowAttrName() { return "nothrow"; } static llvm::StringRef getNoReturnAttrName() { return "noreturn"; } - static llvm::StringRef getSideEffectAttrName() { return "side_effect"; } + static llvm::StringRef getMemoryEffectsAttrName() { + return "memory_effects"; + } static llvm::StringRef getReturnsTwiceAttrName() { return "returns_twice"; } static llvm::StringRef getColdAttrName() { return "cold"; } static llvm::StringRef getHotAttrName() { return "hot"; } @@ -54,6 +56,7 @@ def CIR_Dialect : Dialect { static llvm::StringRef getNoDuplicatesAttrName() { return "noduplicate"; } static llvm::StringRef getConvergentAttrName() { return "convergent"; } static llvm::StringRef getNoUnwindAttrName() { return "nounwind"; } + static llvm::StringRef getWillReturnAttrName() { return "willreturn"; } static llvm::StringRef getModuleLevelAsmAttrName() { return "cir.module_asm"; } static llvm::StringRef getGlobalCtorsAttrName() { return "cir.global_ctors"; } static llvm::StringRef getGlobalDtorsAttrName() { return "cir.global_dtors"; } diff --git a/clang/include/clang/CIR/Dialect/IR/CIROps.td b/clang/include/clang/CIR/Dialect/IR/CIROps.td index b366e781084a4..7051c3b2c2afa 100644 --- a/clang/include/clang/CIR/Dialect/IR/CIROps.td +++ b/clang/include/clang/CIR/Dialect/IR/CIROps.td @@ -4296,7 +4296,7 @@ def CIR_FuncOp : CIR_Op<"func", [ OptionalAttr<DictArrayAttr>:$arg_attrs, OptionalAttr<DictArrayAttr>:$res_attrs, OptionalAttr<FlatSymbolRefAttr>:$aliasee, - OptionalAttr<CIR_SideEffectAttr>:$side_effect, + OptionalAttr<CIR_MemoryEffectsAttr>:$memory_effects, OptionalAttr<FlatSymbolRefAttr>:$personality, CIR_OptionalPriorityAttr:$global_ctor_priority, CIR_OptionalPriorityAttr:$global_dtor_priority, @@ -4598,9 +4598,11 @@ class CIR_CallOpBase<string mnemonic, list<Trait> extra_traits = []> dag commonArgs = (ins OptionalAttr<FlatSymbolRefAttr>:$callee, Variadic<CIR_AnyType>:$args, UnitAttr:$nothrow, + UnitAttr:$nounwind, + UnitAttr:$willreturn, OptionalAttr<CIR_InlineKindAttr>:$inline_kind, UnitAttr:$musttail, - DefaultValuedAttr<CIR_SideEffectAttr, "SideEffect::All">:$side_effect, + OptionalAttr<CIR_MemoryEffectsAttr>:$memory_effects, OptionalAttr<DictArrayAttr>:$arg_attrs, OptionalAttr<DictArrayAttr>:$res_attrs ); @@ -4696,7 +4698,7 @@ def CIR_TryCallOp : CIR_CallOpBase<"try_call",[ "mlir::Block *":$normalDest, "mlir::Block *":$unwindDest, CArg<"mlir::ValueRange", "{}">:$callOperands, - CArg<"SideEffect", "SideEffect::All">:$sideEffect), [{ + CArg<"cir::MemoryEffectsAttr", "{}">:$memoryEffects), [{ $_state.addOperands(callOperands); if (callee) @@ -4704,8 +4706,8 @@ def CIR_TryCallOp : CIR_CallOpBase<"try_call",[ if (resType && !isa<VoidType>(resType)) $_state.addTypes(resType); - $_state.addAttribute("side_effect", - SideEffectAttr::get($_builder.getContext(), sideEffect)); + if (memoryEffects) + $_state.addAttribute("memory_effects", memoryEffects); // Handle branches $_state.addSuccessors(normalDest); @@ -4716,7 +4718,7 @@ def CIR_TryCallOp : CIR_CallOpBase<"try_call",[ "mlir::Block *":$normalDest, "mlir::Block *":$unwindDest, CArg<"mlir::ValueRange", "{}">:$callOperands, - CArg<"SideEffect", "SideEffect::All">:$sideEffect), [{ + CArg<"cir::MemoryEffectsAttr", "{}">:$memoryEffects), [{ ::llvm::SmallVector<mlir::Value, 4> finalCallOperands({ind_target}); finalCallOperands.append(callOperands.begin(), callOperands.end()); $_state.addOperands(finalCallOperands); @@ -4724,8 +4726,8 @@ def CIR_TryCallOp : CIR_CallOpBase<"try_call",[ if (!fn_type.hasVoidReturn()) $_state.addTypes(fn_type.getReturnType()); - $_state.addAttribute("side_effect", - SideEffectAttr::get($_builder.getContext(), sideEffect)); + if (memoryEffects) + $_state.addAttribute("memory_effects", memoryEffects); // Handle branches $_state.addSuccessors(normalDest); diff --git a/clang/include/clang/CIR/Interfaces/CIROpInterfaces.td b/clang/include/clang/CIR/Interfaces/CIROpInterfaces.td index c39a9c7818da7..a759e9c147b44 100644 --- a/clang/include/clang/CIR/Interfaces/CIROpInterfaces.td +++ b/clang/include/clang/CIR/Interfaces/CIROpInterfaces.td @@ -43,8 +43,8 @@ let cppNamespace = "::cir" in { "mlir::Value", "getIndirectCall", (ins)>, InterfaceMethod<"Return whether the callee is nothrow", "bool", "getNothrow", (ins)>, - InterfaceMethod<"Return the side effects of the call operation", - "cir::SideEffect", "getSideEffect", (ins)>, + InterfaceMethod<"Return the memory effects of the call operation", + "cir::MemoryEffectsAttr", "getMemoryEffectsAttr", (ins)>, InterfaceMethod<[{"Set the inline-kind of a call operation"}], "void", "setInlineKind", (ins "std::optional<cir::InlineKind>":$kind), [{}], diff --git a/clang/lib/CIR/CodeGen/CIRGenCall.cpp b/clang/lib/CIR/CodeGen/CIRGenCall.cpp index 2caf22d3beb88..304cf0e8bf3f6 100644 --- a/clang/lib/CIR/CodeGen/CIRGenCall.cpp +++ b/clang/lib/CIR/CodeGen/CIRGenCall.cpp @@ -322,9 +322,8 @@ void CIRGenModule::constructAttributeList( CIRGenCalleeInfo calleeInfo, mlir::NamedAttrList &attrs, llvm::MutableArrayRef<mlir::NamedAttrList> argAttrs, mlir::NamedAttrList &retAttrs, cir::CallingConv &callingConv, - cir::SideEffect &sideEffect, bool attrOnCallSite, bool isThunk) { + bool attrOnCallSite, bool isThunk) { callingConv = info.getCallingConvention(); - sideEffect = cir::SideEffect::All; auto addUnitAttr = [&](llvm::StringRef name) { attrs.set(name, mlir::UnitAttr::get(&getMLIRContext())); @@ -390,19 +389,22 @@ void CIRGenModule::constructAttributeList( assert(!cir::MissingFeatures::opCallAttrs()); - // 'const', 'pure' and 'noalias' attributed functions are also nounwind. - if (targetDecl->hasAttr<ConstAttr>()) { - // gcc specifies that 'const' functions have greater restrictions than - // 'pure' functions, so they also cannot have infinite loops. - sideEffect = cir::SideEffect::Const; - } else if (targetDecl->hasAttr<PureAttr>()) { - // gcc specifies that 'pure' functions cannot have infinite loops. - sideEffect = cir::SideEffect::Pure; + // 'const' and 'pure' imply more than memory effects: the callee also + // cannot unwind and cannot loop forever. Each gets its own attribute, + // since none of the three can be derived from another. + std::optional<cir::ModRefInfo> access; + if (targetDecl->hasAttr<ConstAttr>()) + access = cir::ModRefInfo::NoModRef; + else if (targetDecl->hasAttr<PureAttr>()) + access = cir::ModRefInfo::Ref; + + if (access) { + attrs.set(cir::CIRDialect::getMemoryEffectsAttrName(), + cir::MemoryEffectsAttr::get(&getMLIRContext(), *access)); + addUnitAttr(cir::CIRDialect::getNoUnwindAttrName()); + addUnitAttr(cir::CIRDialect::getWillReturnAttrName()); } - attrs.set(cir::CIRDialect::getSideEffectAttrName(), - cir::SideEffectAttr::get(&getMLIRContext(), sideEffect)); - // TODO(cir): Add noalias to returns for malloc-like functions // (__attribute__((malloc)) / __declspec(restrict)). @@ -1323,9 +1325,8 @@ RValue CIRGenFunction::emitCall(const CIRGenFunctionInfo &funcInfo, assert(!cir::MissingFeatures::opCallCallConv()); assert(!cir::MissingFeatures::opCallAttrs()); cir::CallingConv callingConv; - cir::SideEffect sideEffect; cgm.constructAttributeList(funcName, funcInfo, callee.getAbstractInfo(), - attrs, argAttrs, retAttrs, callingConv, sideEffect, + attrs, argAttrs, retAttrs, callingConv, /*attrOnCallSite=*/true, /*isThunk=*/false); auto resolvedFuncOpFromGlobal = [&](mlir::Operation *op) -> cir::FuncOp { diff --git a/clang/lib/CIR/CodeGen/CIRGenModule.cpp b/clang/lib/CIR/CodeGen/CIRGenModule.cpp index bdc2707723283..bea0f05246efc 100644 --- a/clang/lib/CIR/CodeGen/CIRGenModule.cpp +++ b/clang/lib/CIR/CodeGen/CIRGenModule.cpp @@ -3232,7 +3232,6 @@ void CIRGenModule::setCIRFunctionAttributes(GlobalDecl globalDecl, cir::FuncOp func, bool isThunk) { // TODO(cir): More logic of constructAttributeList is needed. cir::CallingConv callingConv; - cir::SideEffect sideEffect; // TODO(cir): The current list should be initialized with the extra function // attributes, but we don't have those yet. For now, the PAL is initialized @@ -3243,7 +3242,7 @@ void CIRGenModule::setCIRFunctionAttributes(GlobalDecl globalDecl, std::vector<mlir::NamedAttrList> argAttrs(info.arguments().size()); mlir::NamedAttrList retAttrs{}; constructAttributeList(func.getName(), info, globalDecl, pal, argAttrs, - retAttrs, callingConv, sideEffect, + retAttrs, callingConv, /*attrOnCallSite=*/false, isThunk); for (mlir::NamedAttribute attr : pal) diff --git a/clang/lib/CIR/CodeGen/CIRGenModule.h b/clang/lib/CIR/CodeGen/CIRGenModule.h index 51b9c420c94be..9fb74f221fafd 100644 --- a/clang/lib/CIR/CodeGen/CIRGenModule.h +++ b/clang/lib/CIR/CodeGen/CIRGenModule.h @@ -366,8 +366,6 @@ class CIRGenModule : public CIRGenTypeCache { /// contribute to the function attributes and calling convention. /// \param attrs [out] - On return, the attribute list to use. /// \param callingConv [out] - On return, the calling convention to use. - /// \param sideEffect [out] - On return, the side effect type of the - /// attributes. /// \param attrOnCallSite - Whether or not the attributes are on a call site. /// \param isThunk - Whether the function is a thunk. void constructAttributeList( @@ -375,7 +373,7 @@ class CIRGenModule : public CIRGenTypeCache { CIRGenCalleeInfo calleeInfo, mlir::NamedAttrList &attrs, llvm::MutableArrayRef<mlir::NamedAttrList> argAttrs, mlir::NamedAttrList &retAttrs, cir::CallingConv &callingConv, - cir::SideEffect &sideEffect, bool attrOnCallSite, bool isThunk); + bool attrOnCallSite, bool isThunk); /// Helper function for constructAttributeList/others. Builds a set of /// function attributes to add to a function based on language opts, codegen /// opts, and some small properties. diff --git a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp index 23426bed89f6a..043304bd72250 100644 --- a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp +++ b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp @@ -302,7 +302,7 @@ template <typename Ty> struct EnumTraits {}; REGISTER_ENUM_TYPE(GlobalLinkageKind); REGISTER_ENUM_TYPE(VisibilityKind); -REGISTER_ENUM_TYPE(SideEffect); +REGISTER_ENUM_TYPE(ModRefInfo); REGISTER_ENUM_TYPE(CallingConv); } // namespace @@ -335,6 +335,111 @@ static ParseResult parseCIRKeyword(AsmParser &parser, RetTy &result) { return success(); } +/// The memory classes that can be named individually, in the order they +/// print. Positional: parseMemoryEffects and printMemoryEffects both index +/// by position, so a class added here needs an access added alongside it in +/// both. The spellings are LLVM's own, which underscores the target pair +/// but not the rest. +static constexpr llvm::StringLiteral memoryClassNames[] = { + "argmem", "inaccessiblemem", "errnomem", "target_mem0", "target_mem1"}; + +/// Print memory effects in LLVM's compact form, except that the two target +/// classes always print by name rather than collapsing to `target_mem`. The +/// access covering every class not named individually comes first, then only +/// the classes differing from it. That leading access is left out when it is +/// `none` and some class does differ, since an unnamed class reads back as +/// `none`. +static void printMemoryEffects(mlir::AsmPrinter &printer, + cir::MemoryEffectsAttr effects) { + cir::ModRefInfo other = effects.getOther(); + printer << " memory("; + if (effects.isUniform()) { + printer << cir::stringifyModRefInfo(other) << ")"; + return; + } + + bool needComma = false; + if (other != cir::ModRefInfo::NoModRef) { + printer << cir::stringifyModRefInfo(other); + needComma = true; + } + + cir::ModRefInfo classes[] = { + effects.getArgMem(), effects.getInaccessibleMem(), effects.getErrnoMem(), + effects.getTargetMem0(), effects.getTargetMem1()}; + for (auto [name, info] : llvm::zip_equal(memoryClassNames, classes)) { + if (info == other) + continue; + if (needComma) + printer << ", "; + printer << name << ": " << cir::stringifyModRefInfo(info); + needComma = true; + } + printer << ")"; +} + +/// Parse the compact form printed by printMemoryEffects. The opening +/// `memory` keyword has already been consumed. +static mlir::ParseResult parseMemoryEffects(mlir::AsmParser &parser, + cir::MemoryEffectsAttr &result) { + if (parser.parseLParen().failed()) + return failure(); + + // A class left unnamed inherits `other`, which is `none` unless the spelling + // opens with a bare access. + cir::ModRefInfo other = cir::ModRefInfo::NoModRef; + std::array<std::optional<cir::ModRefInfo>, std::size(memoryClassNames)> named; + + // Record one named class, with its name and colon already consumed. + auto parseClass = [&](llvm::SMLoc loc, llvm::StringRef name) -> ParseResult { + const auto *entry = llvm::find(memoryClassNames, name); + if (entry == std::end(memoryClassNames)) + return parser.emitError(loc, "unknown memory class '") << name << "'"; + llvm::SMLoc accessLoc = parser.getCurrentLocation(); + cir::ModRefInfo info; + if (parseCIRKeyword<cir::ModRefInfo>(parser, info).failed()) + return parser.emitError(accessLoc, "expected a memory access kind"); + std::optional<cir::ModRefInfo> &slot = + named[entry - std::begin(memoryClassNames)]; + if (slot) + return parser.emitError(loc, "duplicate memory class '") << name << "'"; + slot = info; + return success(); + }; + + llvm::SMLoc firstLoc = parser.getCurrentLocation(); + llvm::StringRef first; + if (parser.parseKeyword(&first).failed()) + return failure(); + + if (parser.parseOptionalColon().succeeded()) { + if (parseClass(firstLoc, first).failed()) + return failure(); + } else if (std::optional<cir::ModRefInfo> parsed = + cir::symbolizeModRefInfo(first)) { + other = *parsed; + } else { + return parser.emitError(firstLoc, "expected a memory access kind or class"); + } + + while (parser.parseOptionalComma().succeeded()) { + llvm::SMLoc loc = parser.getCurrentLocation(); + llvm::StringRef name; + if (parser.parseKeyword(&name).failed() || parser.parseColon().failed() || + parseClass(loc, name).failed()) + return failure(); + } + + if (parser.parseRParen().failed()) + return failure(); + + result = cir::MemoryEffectsAttr::get( + parser.getContext(), other, named[0].value_or(other), + named[1].value_or(other), named[2].value_or(other), + named[3].value_or(other), named[4].value_or(other)); + return success(); +}... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/223890 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
