[llvm-branch-commits] [mlir] [mlir][Transforms] Dialect conversion: Fix missing source materialization (PR #97903)
https://github.com/zero9178 approved this pull request. https://github.com/llvm/llvm-project/pull/97903 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [mlir] [mlir][Transforms] Dialect conversion: Simplify handling of dropped arguments (PR #97213)
https://github.com/matthias-springer ready_for_review https://github.com/llvm/llvm-project/pull/97213 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [mlir] [mlir][Transforms] Dialect conversion: Simplify handling of dropped arguments (PR #97213)
llvmbot wrote: @llvm/pr-subscribers-mlir Author: Matthias Springer (matthias-springer) Changes This commit simplifies the handling of dropped arguments and updates some dialect conversion documentation that is outdated. When converting a block signature, a `BlockTypeConversionRewrite` object and potentially multiple `ReplaceBlockArgRewrite` are created. During the "commit" phase, uses of the old block arguments are replaced with the new block arguments, but the old implementation was written in an inconsistent way: some block arguments were replaced in `BlockTypeConversionRewrite::commit` and some were replaced in `ReplaceBlockArgRewrite::commit`. The new `BlockTypeConversionRewrite::commit` implementation is much simpler and no longer modifies any IR; that is done only in `ReplaceBlockArgRewrite` now. The `ConvertedArgInfo` data structure is no longer needed. To that end, materializations of dropped arguments are now built in `applySignatureConversion` instead of `materializeLiveConversions`; the latter function no longer has to deal with dropped arguments. Other minor improvements: - Add more comments to `applySignatureConversion`. Note: Error messages around failed materializations for dropped basic block arguments changed slightly. That is because those materializations are now built in `legalizeUnresolvedMaterialization` instead of `legalizeConvertedArgumentTypes`. This commit is in preparation of decoupling argument/source/target materializations from the dialect conversion. This is a re-upload of #96207. --- Full diff: https://github.com/llvm/llvm-project/pull/97213.diff 2 Files Affected: - (modified) mlir/lib/Transforms/Utils/DialectConversion.cpp (+55-118) - (modified) mlir/test/Transforms/test-legalize-type-conversion.mlir (+2-4) ``diff diff --git a/mlir/lib/Transforms/Utils/DialectConversion.cpp b/mlir/lib/Transforms/Utils/DialectConversion.cpp index 1e0afee2373a9..0b552a7e1ca3b 100644 --- a/mlir/lib/Transforms/Utils/DialectConversion.cpp +++ b/mlir/lib/Transforms/Utils/DialectConversion.cpp @@ -432,34 +432,14 @@ class MoveBlockRewrite : public BlockRewrite { Block *insertBeforeBlock; }; -/// This structure contains the information pertaining to an argument that has -/// been converted. -struct ConvertedArgInfo { - ConvertedArgInfo(unsigned newArgIdx, unsigned newArgSize, - Value castValue = nullptr) - : newArgIdx(newArgIdx), newArgSize(newArgSize), castValue(castValue) {} - - /// The start index of in the new argument list that contains arguments that - /// replace the original. - unsigned newArgIdx; - - /// The number of arguments that replaced the original argument. - unsigned newArgSize; - - /// The cast value that was created to cast from the new arguments to the - /// old. This only used if 'newArgSize' > 1. - Value castValue; -}; - /// Block type conversion. This rewrite is partially reflected in the IR. class BlockTypeConversionRewrite : public BlockRewrite { public: - BlockTypeConversionRewrite( - ConversionPatternRewriterImpl &rewriterImpl, Block *block, - Block *origBlock, SmallVector, 1> argInfo, - const TypeConverter *converter) + BlockTypeConversionRewrite(ConversionPatternRewriterImpl &rewriterImpl, + Block *block, Block *origBlock, + const TypeConverter *converter) : BlockRewrite(Kind::BlockTypeConversion, rewriterImpl, block), -origBlock(origBlock), argInfo(argInfo), converter(converter) {} +origBlock(origBlock), converter(converter) {} static bool classof(const IRRewrite *rewrite) { return rewrite->getKind() == Kind::BlockTypeConversion; @@ -479,10 +459,6 @@ class BlockTypeConversionRewrite : public BlockRewrite { /// The original block that was requested to have its signature converted. Block *origBlock; - /// The conversion information for each of the arguments. The information is - /// std::nullopt if the argument was dropped during conversion. - SmallVector, 1> argInfo; - /// The type converter used to convert the arguments. const TypeConverter *converter; }; @@ -691,12 +667,16 @@ class CreateOperationRewrite : public OperationRewrite { /// The type of materialization. enum MaterializationKind { /// This materialization materializes a conversion for an illegal block - /// argument type, to a legal one. + /// argument type, to the original one. Argument, /// This materialization materializes a conversion from an illegal type to a /// legal one. - Target + Target, + + /// This materialization materializes a conversion from a legal type back to + /// an illegal one. + Source }; /// An unresolved materialization, i.e., a "builtin.unrealized_conversion_cast" @@ -736,7 +716,7 @@ class UnresolvedMaterializationRewrite : public OperationRewrite { private: /// The corresponding type converter to use when resolving this /// materialization,
[llvm-branch-commits] [mlir] [mlir][Transforms] Dialect conversion: Simplify handling of dropped arguments (PR #97213)
llvmbot wrote: @llvm/pr-subscribers-mlir-core Author: Matthias Springer (matthias-springer) Changes This commit simplifies the handling of dropped arguments and updates some dialect conversion documentation that is outdated. When converting a block signature, a `BlockTypeConversionRewrite` object and potentially multiple `ReplaceBlockArgRewrite` are created. During the "commit" phase, uses of the old block arguments are replaced with the new block arguments, but the old implementation was written in an inconsistent way: some block arguments were replaced in `BlockTypeConversionRewrite::commit` and some were replaced in `ReplaceBlockArgRewrite::commit`. The new `BlockTypeConversionRewrite::commit` implementation is much simpler and no longer modifies any IR; that is done only in `ReplaceBlockArgRewrite` now. The `ConvertedArgInfo` data structure is no longer needed. To that end, materializations of dropped arguments are now built in `applySignatureConversion` instead of `materializeLiveConversions`; the latter function no longer has to deal with dropped arguments. Other minor improvements: - Add more comments to `applySignatureConversion`. Note: Error messages around failed materializations for dropped basic block arguments changed slightly. That is because those materializations are now built in `legalizeUnresolvedMaterialization` instead of `legalizeConvertedArgumentTypes`. This commit is in preparation of decoupling argument/source/target materializations from the dialect conversion. This is a re-upload of #96207. --- Full diff: https://github.com/llvm/llvm-project/pull/97213.diff 2 Files Affected: - (modified) mlir/lib/Transforms/Utils/DialectConversion.cpp (+55-118) - (modified) mlir/test/Transforms/test-legalize-type-conversion.mlir (+2-4) ``diff diff --git a/mlir/lib/Transforms/Utils/DialectConversion.cpp b/mlir/lib/Transforms/Utils/DialectConversion.cpp index 1e0afee2373a9..0b552a7e1ca3b 100644 --- a/mlir/lib/Transforms/Utils/DialectConversion.cpp +++ b/mlir/lib/Transforms/Utils/DialectConversion.cpp @@ -432,34 +432,14 @@ class MoveBlockRewrite : public BlockRewrite { Block *insertBeforeBlock; }; -/// This structure contains the information pertaining to an argument that has -/// been converted. -struct ConvertedArgInfo { - ConvertedArgInfo(unsigned newArgIdx, unsigned newArgSize, - Value castValue = nullptr) - : newArgIdx(newArgIdx), newArgSize(newArgSize), castValue(castValue) {} - - /// The start index of in the new argument list that contains arguments that - /// replace the original. - unsigned newArgIdx; - - /// The number of arguments that replaced the original argument. - unsigned newArgSize; - - /// The cast value that was created to cast from the new arguments to the - /// old. This only used if 'newArgSize' > 1. - Value castValue; -}; - /// Block type conversion. This rewrite is partially reflected in the IR. class BlockTypeConversionRewrite : public BlockRewrite { public: - BlockTypeConversionRewrite( - ConversionPatternRewriterImpl &rewriterImpl, Block *block, - Block *origBlock, SmallVector, 1> argInfo, - const TypeConverter *converter) + BlockTypeConversionRewrite(ConversionPatternRewriterImpl &rewriterImpl, + Block *block, Block *origBlock, + const TypeConverter *converter) : BlockRewrite(Kind::BlockTypeConversion, rewriterImpl, block), -origBlock(origBlock), argInfo(argInfo), converter(converter) {} +origBlock(origBlock), converter(converter) {} static bool classof(const IRRewrite *rewrite) { return rewrite->getKind() == Kind::BlockTypeConversion; @@ -479,10 +459,6 @@ class BlockTypeConversionRewrite : public BlockRewrite { /// The original block that was requested to have its signature converted. Block *origBlock; - /// The conversion information for each of the arguments. The information is - /// std::nullopt if the argument was dropped during conversion. - SmallVector, 1> argInfo; - /// The type converter used to convert the arguments. const TypeConverter *converter; }; @@ -691,12 +667,16 @@ class CreateOperationRewrite : public OperationRewrite { /// The type of materialization. enum MaterializationKind { /// This materialization materializes a conversion for an illegal block - /// argument type, to a legal one. + /// argument type, to the original one. Argument, /// This materialization materializes a conversion from an illegal type to a /// legal one. - Target + Target, + + /// This materialization materializes a conversion from a legal type back to + /// an illegal one. + Source }; /// An unresolved materialization, i.e., a "builtin.unrealized_conversion_cast" @@ -736,7 +716,7 @@ class UnresolvedMaterializationRewrite : public OperationRewrite { private: /// The corresponding type converter to use when resolving this /// materializa
[llvm-branch-commits] [mlir] [mlir][Transforms][NFC] Dialect Conversion: Move argument materialization logic (PR #98805)
https://github.com/matthias-springer created https://github.com/llvm/llvm-project/pull/98805 This commit moves the argument materialization logic from `legalizeConvertedArgumentTypes` to `legalizeUnresolvedMaterializations`. Before this change: - Argument materializations were created in `legalizeConvertedArgumentTypes` (which used to call `materializeLiveConversions`). After this change: - `legalizeConvertedArgumentTypes` creates a "placeholder" `unrealized_conversion_cast`. - The placeholder `unrealized_conversion_cast` is replaced with an argument materialization (using the type converter) in `legalizeUnresolvedMaterializations`. - All argument and target materializations now take place in the same location (`legalizeUnresolvedMaterializations`). This commit brings us closer towards creating all source/target/argument materializations in one central step, which can then be made optional (and delegated to the user) in the future. (There is one more source materialization step that has not been moved yet.) This commit also consolidates all `build*UnresolvedMaterialization` functions into a single `buildUnresolvedMaterialization` function. This is a re-upload of #96329. >From dac43d6ee0b2231aa64d49650f9b052ced49d3e4 Mon Sep 17 00:00:00 2001 From: Matthias Springer Date: Thu, 20 Jun 2024 17:40:07 +0200 Subject: [PATCH] [mlir][Transforms][NFC] Dialect Conversion: Move argument materialization logic This commit moves the argument materialization logic from `legalizeConvertedArgumentTypes` to `legalizeUnresolvedMaterializations`. Before this change: - Argument materializations were created in `legalizeConvertedArgumentTypes` (which used to call `materializeLiveConversions`). After this change: - `legalizeConvertedArgumentTypes` creates a "placeholder" `unrealized_conversion_cast`. - The placeholder `unrealized_conversion_cast` is replaced with an argument materialization (using the type converter) in `legalizeUnresolvedMaterializations`. - All argument and target materializations now take place in the same location (`legalizeUnresolvedMaterializations`). This commit brings us closer towards creating all source/target/argument materializations in one central step, which can then be made optional (and delegated to the user) in the future. (There is one more source materialization step that has not been moved yet.) This commit also consolidates all `build*UnresolvedMaterialization` functions into a single `buildUnresolvedMaterialization` function. This is a re-upload of #96329. --- .../Transforms/Utils/DialectConversion.cpp| 138 +++--- 1 file changed, 54 insertions(+), 84 deletions(-) diff --git a/mlir/lib/Transforms/Utils/DialectConversion.cpp b/mlir/lib/Transforms/Utils/DialectConversion.cpp index 0b552a7e1ca3b..a2e7570380351 100644 --- a/mlir/lib/Transforms/Utils/DialectConversion.cpp +++ b/mlir/lib/Transforms/Utils/DialectConversion.cpp @@ -53,6 +53,16 @@ static void logFailure(llvm::ScopedPrinter &os, StringRef fmt, Args &&...args) { }); } +/// Helper function that computes an insertion point where the given value is +/// defined and can be used without a dominance violation. +static OpBuilder::InsertPoint computeInsertPoint(Value value) { + Block *insertBlock = value.getParentBlock(); + Block::iterator insertPt = insertBlock->begin(); + if (OpResult inputRes = dyn_cast(value)) +insertPt = ++inputRes.getOwner()->getIterator(); + return OpBuilder::InsertPoint(insertBlock, insertPt); +} + //===--===// // ConversionValueMapping //===--===// @@ -445,11 +455,9 @@ class BlockTypeConversionRewrite : public BlockRewrite { return rewrite->getKind() == Kind::BlockTypeConversion; } - /// Materialize any necessary conversions for converted arguments that have - /// live users, using the provided `findLiveUser` to search for a user that - /// survives the conversion process. - LogicalResult - materializeLiveConversions(function_ref findLiveUser); + Block *getOrigBlock() const { return origBlock; } + + const TypeConverter *getConverter() const { return converter; } void commit(RewriterBase &rewriter) override; @@ -830,15 +838,10 @@ struct ConversionPatternRewriterImpl : public RewriterBase::Listener { /// Build an unresolved materialization operation given an output type and set /// of input operands. Value buildUnresolvedMaterialization(MaterializationKind kind, - Block *insertBlock, - Block::iterator insertPt, Location loc, + OpBuilder::InsertPoint ip, Location loc, ValueRange inputs, Type outputType, const TypeConverter *converter); - Value buildUnresolvedTargetMaterialization(Locat
[llvm-branch-commits] [mlir] [mlir][Transforms][NFC] Dialect Conversion: Move argument materialization logic (PR #98805)
llvmbot wrote: @llvm/pr-subscribers-mlir-core Author: Matthias Springer (matthias-springer) Changes This commit moves the argument materialization logic from `legalizeConvertedArgumentTypes` to `legalizeUnresolvedMaterializations`. Before this change: - Argument materializations were created in `legalizeConvertedArgumentTypes` (which used to call `materializeLiveConversions`). After this change: - `legalizeConvertedArgumentTypes` creates a "placeholder" `unrealized_conversion_cast`. - The placeholder `unrealized_conversion_cast` is replaced with an argument materialization (using the type converter) in `legalizeUnresolvedMaterializations`. - All argument and target materializations now take place in the same location (`legalizeUnresolvedMaterializations`). This commit brings us closer towards creating all source/target/argument materializations in one central step, which can then be made optional (and delegated to the user) in the future. (There is one more source materialization step that has not been moved yet.) This commit also consolidates all `build*UnresolvedMaterialization` functions into a single `buildUnresolvedMaterialization` function. This is a re-upload of #96329. --- Full diff: https://github.com/llvm/llvm-project/pull/98805.diff 1 Files Affected: - (modified) mlir/lib/Transforms/Utils/DialectConversion.cpp (+54-84) ``diff diff --git a/mlir/lib/Transforms/Utils/DialectConversion.cpp b/mlir/lib/Transforms/Utils/DialectConversion.cpp index 0b552a7e1ca3b..a2e7570380351 100644 --- a/mlir/lib/Transforms/Utils/DialectConversion.cpp +++ b/mlir/lib/Transforms/Utils/DialectConversion.cpp @@ -53,6 +53,16 @@ static void logFailure(llvm::ScopedPrinter &os, StringRef fmt, Args &&...args) { }); } +/// Helper function that computes an insertion point where the given value is +/// defined and can be used without a dominance violation. +static OpBuilder::InsertPoint computeInsertPoint(Value value) { + Block *insertBlock = value.getParentBlock(); + Block::iterator insertPt = insertBlock->begin(); + if (OpResult inputRes = dyn_cast(value)) +insertPt = ++inputRes.getOwner()->getIterator(); + return OpBuilder::InsertPoint(insertBlock, insertPt); +} + //===--===// // ConversionValueMapping //===--===// @@ -445,11 +455,9 @@ class BlockTypeConversionRewrite : public BlockRewrite { return rewrite->getKind() == Kind::BlockTypeConversion; } - /// Materialize any necessary conversions for converted arguments that have - /// live users, using the provided `findLiveUser` to search for a user that - /// survives the conversion process. - LogicalResult - materializeLiveConversions(function_ref findLiveUser); + Block *getOrigBlock() const { return origBlock; } + + const TypeConverter *getConverter() const { return converter; } void commit(RewriterBase &rewriter) override; @@ -830,15 +838,10 @@ struct ConversionPatternRewriterImpl : public RewriterBase::Listener { /// Build an unresolved materialization operation given an output type and set /// of input operands. Value buildUnresolvedMaterialization(MaterializationKind kind, - Block *insertBlock, - Block::iterator insertPt, Location loc, + OpBuilder::InsertPoint ip, Location loc, ValueRange inputs, Type outputType, const TypeConverter *converter); - Value buildUnresolvedTargetMaterialization(Location loc, Value input, - Type outputType, - const TypeConverter *converter); - //======// // Rewriter Notification Hooks //======// @@ -970,49 +973,6 @@ void BlockTypeConversionRewrite::rollback() { block->replaceAllUsesWith(origBlock); } -LogicalResult BlockTypeConversionRewrite::materializeLiveConversions( -function_ref findLiveUser) { - // Process the remapping for each of the original arguments. - for (auto it : llvm::enumerate(origBlock->getArguments())) { -BlockArgument origArg = it.value(); -// Note: `block` may be detached, so OpBuilder::atBlockBegin cannot be used. -OpBuilder builder(it.value().getContext(), /*listener=*/&rewriterImpl); -builder.setInsertionPointToStart(block); - -// If the type of this argument changed and the argument is still live, we -// need to materialize a conversion. -if (rewriterImpl.mapping.lookupOrNull(origArg, origArg.getType())) - continue; -Operation *liveUser = findLiveUser(origArg); -if (!liveUser) - continue; - -Value replacement
[llvm-branch-commits] [mlir] [mlir][Transforms][NFC] Dialect Conversion: Move argument materialization logic (PR #98805)
llvmbot wrote: @llvm/pr-subscribers-mlir Author: Matthias Springer (matthias-springer) Changes This commit moves the argument materialization logic from `legalizeConvertedArgumentTypes` to `legalizeUnresolvedMaterializations`. Before this change: - Argument materializations were created in `legalizeConvertedArgumentTypes` (which used to call `materializeLiveConversions`). After this change: - `legalizeConvertedArgumentTypes` creates a "placeholder" `unrealized_conversion_cast`. - The placeholder `unrealized_conversion_cast` is replaced with an argument materialization (using the type converter) in `legalizeUnresolvedMaterializations`. - All argument and target materializations now take place in the same location (`legalizeUnresolvedMaterializations`). This commit brings us closer towards creating all source/target/argument materializations in one central step, which can then be made optional (and delegated to the user) in the future. (There is one more source materialization step that has not been moved yet.) This commit also consolidates all `build*UnresolvedMaterialization` functions into a single `buildUnresolvedMaterialization` function. This is a re-upload of #96329. --- Full diff: https://github.com/llvm/llvm-project/pull/98805.diff 1 Files Affected: - (modified) mlir/lib/Transforms/Utils/DialectConversion.cpp (+54-84) ``diff diff --git a/mlir/lib/Transforms/Utils/DialectConversion.cpp b/mlir/lib/Transforms/Utils/DialectConversion.cpp index 0b552a7e1ca3b..a2e7570380351 100644 --- a/mlir/lib/Transforms/Utils/DialectConversion.cpp +++ b/mlir/lib/Transforms/Utils/DialectConversion.cpp @@ -53,6 +53,16 @@ static void logFailure(llvm::ScopedPrinter &os, StringRef fmt, Args &&...args) { }); } +/// Helper function that computes an insertion point where the given value is +/// defined and can be used without a dominance violation. +static OpBuilder::InsertPoint computeInsertPoint(Value value) { + Block *insertBlock = value.getParentBlock(); + Block::iterator insertPt = insertBlock->begin(); + if (OpResult inputRes = dyn_cast(value)) +insertPt = ++inputRes.getOwner()->getIterator(); + return OpBuilder::InsertPoint(insertBlock, insertPt); +} + //===--===// // ConversionValueMapping //===--===// @@ -445,11 +455,9 @@ class BlockTypeConversionRewrite : public BlockRewrite { return rewrite->getKind() == Kind::BlockTypeConversion; } - /// Materialize any necessary conversions for converted arguments that have - /// live users, using the provided `findLiveUser` to search for a user that - /// survives the conversion process. - LogicalResult - materializeLiveConversions(function_ref findLiveUser); + Block *getOrigBlock() const { return origBlock; } + + const TypeConverter *getConverter() const { return converter; } void commit(RewriterBase &rewriter) override; @@ -830,15 +838,10 @@ struct ConversionPatternRewriterImpl : public RewriterBase::Listener { /// Build an unresolved materialization operation given an output type and set /// of input operands. Value buildUnresolvedMaterialization(MaterializationKind kind, - Block *insertBlock, - Block::iterator insertPt, Location loc, + OpBuilder::InsertPoint ip, Location loc, ValueRange inputs, Type outputType, const TypeConverter *converter); - Value buildUnresolvedTargetMaterialization(Location loc, Value input, - Type outputType, - const TypeConverter *converter); - //======// // Rewriter Notification Hooks //======// @@ -970,49 +973,6 @@ void BlockTypeConversionRewrite::rollback() { block->replaceAllUsesWith(origBlock); } -LogicalResult BlockTypeConversionRewrite::materializeLiveConversions( -function_ref findLiveUser) { - // Process the remapping for each of the original arguments. - for (auto it : llvm::enumerate(origBlock->getArguments())) { -BlockArgument origArg = it.value(); -// Note: `block` may be detached, so OpBuilder::atBlockBegin cannot be used. -OpBuilder builder(it.value().getContext(), /*listener=*/&rewriterImpl); -builder.setInsertionPointToStart(block); - -// If the type of this argument changed and the argument is still live, we -// need to materialize a conversion. -if (rewriterImpl.mapping.lookupOrNull(origArg, origArg.getType())) - continue; -Operation *liveUser = findLiveUser(origArg); -if (!liveUser) - continue; - -Value replacementValue
[llvm-branch-commits] [llvm] [BOLT] Support POSSIBLE_PIC_FIXED_BRANCH (PR #91667)
https://github.com/aaupov updated https://github.com/llvm/llvm-project/pull/91667 >From dd4d0de42048c063d5e5095a0c2594c7cc578df5 Mon Sep 17 00:00:00 2001 From: Amir Ayupov Date: Thu, 9 May 2024 19:35:26 -0700 Subject: [PATCH 1/4] Fix RISCVMCPlusBuilder Created using spr 1.3.4 --- bolt/lib/Target/RISCV/RISCVMCPlusBuilder.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/bolt/lib/Target/RISCV/RISCVMCPlusBuilder.cpp b/bolt/lib/Target/RISCV/RISCVMCPlusBuilder.cpp index 74f2f0aae91e6..020e62463ee2f 100644 --- a/bolt/lib/Target/RISCV/RISCVMCPlusBuilder.cpp +++ b/bolt/lib/Target/RISCV/RISCVMCPlusBuilder.cpp @@ -177,13 +177,14 @@ class RISCVMCPlusBuilder : public MCPlusBuilder { MCInst &Instruction, InstructionIterator Begin, InstructionIterator End, const unsigned PtrSize, MCInst *&MemLocInstr, unsigned &BaseRegNum, unsigned &IndexRegNum, int64_t &DispValue, const MCExpr *&DispExpr, - MCInst *&PCRelBaseOut) const override { + MCInst *&PCRelBaseOut, MCInst *&FixedEntryLoadInst) const override { MemLocInstr = nullptr; BaseRegNum = 0; IndexRegNum = 0; DispValue = 0; DispExpr = nullptr; PCRelBaseOut = nullptr; +FixedEntryLoadInst = nullptr; // Check for the following long tail call sequence: // 1: auipc xi, %pcrel_hi(sym) >From 62391bb5aa01f2b77d4315d1e72a9924eec9ecc0 Mon Sep 17 00:00:00 2001 From: Amir Ayupov Date: Fri, 5 Jul 2024 14:54:51 -0700 Subject: [PATCH 2/4] Drop deregisterJumpTable Created using spr 1.3.4 --- bolt/lib/Core/BinaryFunction.cpp | 10 +- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/bolt/lib/Core/BinaryFunction.cpp b/bolt/lib/Core/BinaryFunction.cpp index 09a6ca1d68730..f587d5a2cadd4 100644 --- a/bolt/lib/Core/BinaryFunction.cpp +++ b/bolt/lib/Core/BinaryFunction.cpp @@ -899,17 +899,9 @@ BinaryFunction::processIndirectBranch(MCInst &Instruction, unsigned Size, TargetAddress = ArrayStart + *Value; -// Remove spurious JumpTable at EntryAddress caused by PIC reference from -// the load instruction. -JumpTable *JT = BC.getJumpTableContainingAddress(EntryAddress); -assert(JT && "Must have a jump table at fixed entry address"); -BC.deregisterJumpTable(EntryAddress); -JumpTables.erase(EntryAddress); -delete JT; - // Replace FixedEntryDispExpr used in target address calculation with outer // jump table reference. -JT = BC.getJumpTableContainingAddress(ArrayStart); +JumpTable *JT = BC.getJumpTableContainingAddress(ArrayStart); assert(JT && "Must have a containing jump table for PIC fixed branch"); BC.MIB->replaceMemOperandDisp(*FixedEntryLoadInstr, JT->getFirstLabel(), EntryAddress - ArrayStart, &*BC.Ctx); >From 5336879ab68aedb1217e2c6c139d171f31e89e03 Mon Sep 17 00:00:00 2001 From: Amir Ayupov Date: Sat, 6 Jul 2024 22:26:14 -0700 Subject: [PATCH 3/4] Surgically drop spurious jump table Created using spr 1.3.4 --- bolt/include/bolt/Core/BinaryContext.h | 5 + bolt/lib/Core/BinaryFunction.cpp| 12 ++-- bolt/test/X86/jump-table-fixed-ref-pic.test | 11 --- 3 files changed, 19 insertions(+), 9 deletions(-) diff --git a/bolt/include/bolt/Core/BinaryContext.h b/bolt/include/bolt/Core/BinaryContext.h index 73932c4ca2fb3..c5e2c6cd02179 100644 --- a/bolt/include/bolt/Core/BinaryContext.h +++ b/bolt/include/bolt/Core/BinaryContext.h @@ -431,6 +431,11 @@ class BinaryContext { return nullptr; } + /// Deregister JumpTable registered at a given \p Address. + bool deregisterJumpTable(uint64_t Address) { +return JumpTables.erase(Address); + } + unsigned getDWARFEncodingSize(unsigned Encoding) { if (Encoding == dwarf::DW_EH_PE_omit) return 0; diff --git a/bolt/lib/Core/BinaryFunction.cpp b/bolt/lib/Core/BinaryFunction.cpp index f587d5a2cadd4..2ecca32a5985c 100644 --- a/bolt/lib/Core/BinaryFunction.cpp +++ b/bolt/lib/Core/BinaryFunction.cpp @@ -899,9 +899,17 @@ BinaryFunction::processIndirectBranch(MCInst &Instruction, unsigned Size, TargetAddress = ArrayStart + *Value; +// Remove spurious JumpTable at EntryAddress caused by PIC reference from +// the load instruction. +JumpTable *JT = BC.getJumpTableContainingAddress(EntryAddress); +assert(JT && "Must have a jump table at fixed entry address"); +BC.deregisterJumpTable(EntryAddress); +JumpTables.erase(EntryAddress); +delete JT; + // Replace FixedEntryDispExpr used in target address calculation with outer // jump table reference. -JumpTable *JT = BC.getJumpTableContainingAddress(ArrayStart); +JT = BC.getJumpTableContainingAddress(ArrayStart); assert(JT && "Must have a containing jump table for PIC fixed branch"); BC.MIB->replaceMemOperandDisp(*FixedEntryLoadInstr, JT->getFirstLabel(), EntryAddress - ArrayStart, &*BC.Ctx); @@ -1158,10 +1166,10 @@ void BinaryFunction:
[llvm-branch-commits] [flang] [Flang][OpenMP] Derived type explicit allocatable member mapping (PR #96266)
https://github.com/ergawy edited https://github.com/llvm/llvm-project/pull/96266 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [flang] [Flang][OpenMP] Derived type explicit allocatable member mapping (PR #96266)
@@ -52,12 +52,22 @@ using DeclareTargetCapturePair = struct OmpMapMemberIndicesData { // The indices representing the component members placement in its derived // type parents hierarchy. - llvm::SmallVector memberPlacementIndices; + llvm::SmallVector> memberPlacementIndices; // Placement of the member in the member vector. - mlir::omp::MapInfoOp memberMap; + llvm::SmallVector memberMap; }; +llvm::SmallVector +generateMemberPlacementIndices(const Object &object, ergawy wrote: There is already a version of `generateMemberPlacementIndices` that takes a `SmallVector` param as an output param. And both versions have exactly the same logic. Is there a reason for duplication? https://github.com/llvm/llvm-project/pull/96266 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [flang] [Flang][OpenMP] Derived type explicit allocatable member mapping (PR #96266)
@@ -953,6 +954,22 @@ bool ClauseProcessor::processMap( if (origSymbol && fir::isTypeWithDescriptor(origSymbol.getType())) symAddr = origSymbol; + if (object.sym()->owner().IsDerivedType()) { +omp::ObjectList objectList = gatherObjects(object, semaCtx); +parentObj = objectList[0]; ergawy wrote: ```suggestion assert(!objectList.empty()); parentObj = objectList[0]; ``` https://github.com/llvm/llvm-project/pull/96266 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [flang] [Flang][OpenMP] Derived type explicit allocatable member mapping (PR #96266)
@@ -52,12 +52,22 @@ using DeclareTargetCapturePair = struct OmpMapMemberIndicesData { ergawy wrote: It would be nice to extend the doc for this `struct` with some examples where each example consists of: - some Fortran variable the user wants to map, - and how the `OmpMapMemberIndicesData` looks like for that variable. https://github.com/llvm/llvm-project/pull/96266 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [flang] [Flang][OpenMP] Derived type explicit allocatable member mapping (PR #96266)
@@ -671,4 +672,51 @@ static inline bool isEqual(const Fortran::lower::SomeExpr *x, } } // end namespace Fortran::lower +// OpenMP utility functions used in locations outside of the +// OpenMP lowering. +namespace Fortran::lower::omp { + +[[maybe_unused]] static void fillMemberIndices( ergawy wrote: To reduce compilation times of files including this one, can you move the implementations of these 2 utils to a corresponding `.cpp` file? https://github.com/llvm/llvm-project/pull/96266 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [flang] [Flang][OpenMP] Derived type explicit allocatable member mapping (PR #96266)
@@ -671,4 +672,51 @@ static inline bool isEqual(const Fortran::lower::SomeExpr *x, } } // end namespace Fortran::lower +// OpenMP utility functions used in locations outside of the +// OpenMP lowering. +namespace Fortran::lower::omp { + +[[maybe_unused]] static void fillMemberIndices( +llvm::SmallVector> &memberPlacementData) { + size_t largestIndicesSize = + std::max_element(memberPlacementData.begin(), memberPlacementData.end(), + [](auto a, auto b) { return a.size() < b.size(); }) + ->size(); + + // DenseElementsAttr expects a rectangular shape for the data, so all + // index lists have to be of the same length, this emplaces -1 as filler. ergawy wrote: Does the `-1` value has a significance here or is it just a random value? https://github.com/llvm/llvm-project/pull/96266 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [flang] [Flang][OpenMP] Derived type explicit allocatable member mapping (PR #96266)
@@ -30,17 +30,17 @@ subroutine mapType_array !$omp end target end subroutine mapType_array -!CHECK: @.offload_sizes{{.*}} = private unnamed_addr constant [3 x i64] [i64 0, i64 24, i64 4] -!CHECK: @.offload_maptypes{{.*}} = private unnamed_addr constant [3 x i64] [i64 32, i64 281474976710657, i64 281474976711187] +!CHECK: @.offload_sizes{{.*}} = private unnamed_addr constant [4 x i64] [i64 0, i64 24, i64 8, i64 4] ergawy wrote: The test fails on my machine on that line. Does it do the same for you as well? In general, it would be nice to have comments before each `CHECK` line to explain these magic numbers; why is 3 or 4 or whatever. https://github.com/llvm/llvm-project/pull/96266 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [flang] [Flang][OpenMP] Derived type explicit allocatable member mapping (PR #96266)
https://github.com/ergawy commented: Thanks for the PR @agozillon. I did a first round of review and a few suggestions. I also have a few questions that might be obvious, so please excuse any newbie-like questions here :D. https://github.com/llvm/llvm-project/pull/96266 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [flang] [Flang][OpenMP] Derived type explicit allocatable member mapping (PR #96266)
@@ -671,4 +672,51 @@ static inline bool isEqual(const Fortran::lower::SomeExpr *x, } } // end namespace Fortran::lower +// OpenMP utility functions used in locations outside of the +// OpenMP lowering. +namespace Fortran::lower::omp { + +[[maybe_unused]] static void fillMemberIndices( +llvm::SmallVector> &memberPlacementData) { + size_t largestIndicesSize = + std::max_element(memberPlacementData.begin(), memberPlacementData.end(), ergawy wrote: ```suggestion llvm::max_element(memberPlacementData, ``` https://github.com/llvm/llvm-project/pull/96266 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [flang] [Flang][OpenMP] Derived type explicit allocatable member mapping (PR #96266)
@@ -141,6 +143,110 @@ createMapInfoOp(fir::FirOpBuilder &builder, mlir::Location loc, return op; } +omp::ObjectList gatherObjects(omp::Object obj, + semantics::SemanticsContext &semaCtx) { + omp::ObjectList objList; + std::optional baseObj = getBaseObject(obj, semaCtx); + while (baseObj.has_value()) { +objList.push_back(baseObj.value()); +baseObj = getBaseObject(baseObj.value(), semaCtx); + } + return omp::ObjectList{llvm::reverse(objList)}; +} + +bool duplicateMemberMapInfo(OmpMapMemberIndicesData &parentMembers, +llvm::SmallVectorImpl &memberIndices) { + // A variation of std:equal that supports non-equal length index lists for our + // specific use-case, if one is larger than the other, we use -1, the default + // filler element in place of the smaller vector, this prevents UB from over + // indexing and removes the need for us to do any filling of intermediate + // index lists we'll discard. + auto isEqual = [](auto first1, auto last1, auto first2, auto last2) { +int v1, v2; +for (; first1 != last1; ++first1, ++first2) { + v1 = (first1 == last1) ? -1 : *first1; + v2 = (first2 == last2) ? -1 : *first2; + + if (!(v1 == v2)) +return false; +} +return true; + }; + + for (auto memberData : parentMembers.memberPlacementIndices) +if (isEqual(memberData.begin(), memberData.end(), memberIndices.begin(), +memberIndices.end())) + return true; + return false; +} + +// When mapping members of derived types, there is a chance that one of the +// members along the way to a mapped member is an descriptor. In which case +// we have to make sure we generate a map for those along the way otherwise +// we will be missing a chunk of data required to actually map the member +// type to device. This function effectively generates these maps and the +// appropriate data accesses required to generate these maps. It will avoid +// creating duplicate maps, as duplicates are just as bad as unmapped +// descriptor data in a lot of cases for the runtime (and unnecessary +// data movement should be avoided where possible) +mlir::Value createParentSymAndGenIntermediateMaps( +mlir::Location clauseLocation, Fortran::lower::AbstractConverter &converter, +omp::ObjectList &objectList, llvm::SmallVector &indices, +OmpMapMemberIndicesData &parentMemberIndices, std::string asFortran, +llvm::omp::OpenMPOffloadMappingFlags mapTypeBits) { + fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder(); + Fortran::lower::AddrAndBoundsInfo parentBaseAddr = + Fortran::lower::getDataOperandBaseAddr( + converter, firOpBuilder, *objectList[0].sym(), clauseLocation); + mlir::Value curValue = parentBaseAddr.addr; + + for (size_t i = 0; i < objectList.size(); ++i) { +mlir::Type unwrappedTy = +fir::unwrapSequenceType(fir::unwrapPassByRefType(curValue.getType())); +if (fir::RecordType recordType = +mlir::dyn_cast_or_null(unwrappedTy)) { + mlir::Value idxConst = firOpBuilder.createIntegerConstant( + clauseLocation, firOpBuilder.getIndexType(), indices[i]); + mlir::Type memberTy = recordType.getTypeList().at(indices[i]).second; + curValue = firOpBuilder.create( + clauseLocation, firOpBuilder.getRefType(memberTy), curValue, + idxConst); + + if ((i != indices.size() - 1) && fir::isTypeWithDescriptor(memberTy)) { ergawy wrote: ```suggestion if ((i == indices.size() - 1) || !fir::isTypeWithDescriptor(memberTy)) { continue; } ``` https://github.com/llvm/llvm-project/pull/96266 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [flang] [Flang][OpenMP] Derived type explicit allocatable member mapping (PR #96266)
@@ -141,6 +143,110 @@ createMapInfoOp(fir::FirOpBuilder &builder, mlir::Location loc, return op; } +omp::ObjectList gatherObjects(omp::Object obj, + semantics::SemanticsContext &semaCtx) { + omp::ObjectList objList; + std::optional baseObj = getBaseObject(obj, semaCtx); + while (baseObj.has_value()) { +objList.push_back(baseObj.value()); +baseObj = getBaseObject(baseObj.value(), semaCtx); + } + return omp::ObjectList{llvm::reverse(objList)}; +} + +bool duplicateMemberMapInfo(OmpMapMemberIndicesData &parentMembers, +llvm::SmallVectorImpl &memberIndices) { + // A variation of std:equal that supports non-equal length index lists for our + // specific use-case, if one is larger than the other, we use -1, the default + // filler element in place of the smaller vector, this prevents UB from over + // indexing and removes the need for us to do any filling of intermediate + // index lists we'll discard. + auto isEqual = [](auto first1, auto last1, auto first2, auto last2) { +int v1, v2; +for (; first1 != last1; ++first1, ++first2) { ergawy wrote: I think this works correctly only if rang 1 is shorter than range 2, right? Is this a pre-condition for `duplicateMemberMapInfo`? https://github.com/llvm/llvm-project/pull/96266 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [flang] [Flang][OpenMP] Derived type explicit allocatable member mapping (PR #96266)
@@ -953,6 +954,22 @@ bool ClauseProcessor::processMap( if (origSymbol && fir::isTypeWithDescriptor(origSymbol.getType())) symAddr = origSymbol; + if (object.sym()->owner().IsDerivedType()) { +omp::ObjectList objectList = gatherObjects(object, semaCtx); +parentObj = objectList[0]; +parentMemberIndices.insert({parentObj.value(), {}}); +if (Fortran::semantics::IsAllocatableOrObjectPointer( +object.sym()) || +memberHasAllocatableParent(object, semaCtx)) { ergawy wrote: ```suggestion if (isMemberOrParentAllocatableOrPointer(object.sym())) { ``` https://github.com/llvm/llvm-project/pull/96266 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [flang] [Flang][OpenMP] Derived type explicit allocatable member mapping (PR #96266)
@@ -189,85 +295,60 @@ generateMemberPlacementIndices(const Object &object, indices = llvm::SmallVector{llvm::reverse(indices)}; } -void addChildIndexAndMapToParent( -const omp::Object &object, -std::map> &parentMemberIndices, -mlir::omp::MapInfoOp &mapOp, semantics::SemanticsContext &semaCtx) { - std::optional dataRef = ExtractDataRef(object.ref()); - assert(dataRef.has_value() && - "DataRef could not be extracted during mapping of derived type " - "cannot proceed"); - const semantics::Symbol *parentSym = &dataRef->GetFirstSymbol(); - assert(parentSym && "Could not find parent symbol during lower of " - "a component member in OpenMP map clause"); +void addChildIndexAndMapToParent(const omp::Object &object, + OmpMapMemberIndicesData &parentMemberIndices, + mlir::omp::MapInfoOp &mapOp, + semantics::SemanticsContext &semaCtx) { llvm::SmallVector indices; generateMemberPlacementIndices(object, indices, semaCtx); - parentMemberIndices[parentSym].push_back({indices, mapOp}); + parentMemberIndices.memberPlacementIndices.push_back(indices); + parentMemberIndices.memberMap.push_back(mapOp); } -static void calculateShapeAndFillIndices( -llvm::SmallVectorImpl &shape, -llvm::SmallVectorImpl &memberPlacementData) { - shape.push_back(memberPlacementData.size()); - size_t largestIndicesSize = - std::max_element(memberPlacementData.begin(), memberPlacementData.end(), - [](auto a, auto b) { - return a.memberPlacementIndices.size() < -b.memberPlacementIndices.size(); - }) - ->memberPlacementIndices.size(); - shape.push_back(largestIndicesSize); - - // DenseElementsAttr expects a rectangular shape for the data, so all - // index lists have to be of the same length, this emplaces -1 as filler. - for (auto &v : memberPlacementData) { -if (v.memberPlacementIndices.size() < largestIndicesSize) { - auto *prevEnd = v.memberPlacementIndices.end(); - v.memberPlacementIndices.resize(largestIndicesSize); - std::fill(prevEnd, v.memberPlacementIndices.end(), -1); -} +llvm::SmallVector +generateMemberPlacementIndices(const Object &object, + Fortran::semantics::SemanticsContext &semaCtx) { + std::list indices; + auto compObj = getComponentObject(object, semaCtx); + while (compObj) { +indices.push_front(getComponentPlacementInParent(compObj->sym())); +compObj = +getComponentObject(getBaseObject(compObj.value(), semaCtx), semaCtx); } + + return llvm::SmallVector{std::begin(indices), std::end(indices)}; } -static mlir::DenseIntElementsAttr createDenseElementsAttrFromIndices( -llvm::SmallVectorImpl &memberPlacementData, -fir::FirOpBuilder &builder) { - llvm::SmallVector shape; - calculateShapeAndFillIndices(shape, memberPlacementData); - - llvm::SmallVector indicesFlattened = - std::accumulate(memberPlacementData.begin(), memberPlacementData.end(), - llvm::SmallVector(), - [](llvm::SmallVector &x, OmpMapMemberIndicesData y) { -x.insert(x.end(), y.memberPlacementIndices.begin(), - y.memberPlacementIndices.end()); -return x; - }); - - return mlir::DenseIntElementsAttr::get( - mlir::VectorType::get(shape, -mlir::IntegerType::get(builder.getContext(), 32)), - indicesFlattened); +bool memberHasAllocatableParent(const Object &object, +Fortran::semantics::SemanticsContext &semaCtx) { ergawy wrote: ```suggestion bool isMemberOrParentAllocatableOrPointer(const Object &object, Fortran::semantics::SemanticsContext &semaCtx) { if (Fortran::semantics::IsAllocatableOrObjectPoint(object.sym())) return true; ``` https://github.com/llvm/llvm-project/pull/96266 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
[llvm-branch-commits] [flang] [Flang][OpenMP] Derived type explicit allocatable member mapping (PR #96266)
@@ -141,6 +143,110 @@ createMapInfoOp(fir::FirOpBuilder &builder, mlir::Location loc, return op; } +omp::ObjectList gatherObjects(omp::Object obj, + semantics::SemanticsContext &semaCtx) { + omp::ObjectList objList; + std::optional baseObj = getBaseObject(obj, semaCtx); + while (baseObj.has_value()) { +objList.push_back(baseObj.value()); +baseObj = getBaseObject(baseObj.value(), semaCtx); + } + return omp::ObjectList{llvm::reverse(objList)}; +} + +bool duplicateMemberMapInfo(OmpMapMemberIndicesData &parentMembers, +llvm::SmallVectorImpl &memberIndices) { + // A variation of std:equal that supports non-equal length index lists for our + // specific use-case, if one is larger than the other, we use -1, the default + // filler element in place of the smaller vector, this prevents UB from over + // indexing and removes the need for us to do any filling of intermediate + // index lists we'll discard. + auto isEqual = [](auto first1, auto last1, auto first2, auto last2) { +int v1, v2; +for (; first1 != last1; ++first1, ++first2) { + v1 = (first1 == last1) ? -1 : *first1; + v2 = (first2 == last2) ? -1 : *first2; + + if (!(v1 == v2)) +return false; +} +return true; + }; + + for (auto memberData : parentMembers.memberPlacementIndices) +if (isEqual(memberData.begin(), memberData.end(), memberIndices.begin(), +memberIndices.end())) + return true; + return false; +} + +// When mapping members of derived types, there is a chance that one of the +// members along the way to a mapped member is an descriptor. In which case +// we have to make sure we generate a map for those along the way otherwise +// we will be missing a chunk of data required to actually map the member +// type to device. This function effectively generates these maps and the +// appropriate data accesses required to generate these maps. It will avoid +// creating duplicate maps, as duplicates are just as bad as unmapped +// descriptor data in a lot of cases for the runtime (and unnecessary +// data movement should be avoided where possible) +mlir::Value createParentSymAndGenIntermediateMaps( +mlir::Location clauseLocation, Fortran::lower::AbstractConverter &converter, +omp::ObjectList &objectList, llvm::SmallVector &indices, +OmpMapMemberIndicesData &parentMemberIndices, std::string asFortran, +llvm::omp::OpenMPOffloadMappingFlags mapTypeBits) { + fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder(); ergawy wrote: ```suggestion assert(objectList.size() == indices.size()); fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder(); ``` https://github.com/llvm/llvm-project/pull/96266 ___ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits