From 89b5d88fb81362b4fb2f833790aa40b7eaa186da Mon Sep 17 00:00:00 2001 From: Kazu Hirata Date: Sat, 26 Oct 2024 19:13:56 -0700 Subject: [PATCH 1/9] [ADT] Use std::string_view inside StringRef (#113775) This patch makes minimum changes to replace Data and Length with an instance of std::string_view. Previously, I opted for public inheritance (#113752), but I encountered a lot of errors from gcc stemming from ambiguity between std::string_view and StringRef. The composition approach in this patch gives us greater control at the expense of forwarder functions. --- llvm/include/llvm/ADT/StringRef.h | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/llvm/include/llvm/ADT/StringRef.h b/llvm/include/llvm/ADT/StringRef.h index d5f30b88c4c6a2..f879bbf7164fd6 100644 --- a/llvm/include/llvm/ADT/StringRef.h +++ b/llvm/include/llvm/ADT/StringRef.h @@ -60,11 +60,7 @@ namespace llvm { using const_reverse_iterator = std::reverse_iterator; private: - /// The start of the string, in an external buffer. - const char *Data = nullptr; - - /// The length of the string. - size_t Length = 0; + std::string_view View; // Workaround memcmp issue with null pointers (undefined behavior) // by providing a specialized version @@ -86,27 +82,25 @@ namespace llvm { /// Construct a string ref from a cstring. /*implicit*/ constexpr StringRef(const char *Str) - : Data(Str), Length(Str ? + : View(Str, Str ? // GCC 7 doesn't have constexpr char_traits. Fall back to __builtin_strlen. #if defined(_GLIBCXX_RELEASE) && _GLIBCXX_RELEASE < 8 - __builtin_strlen(Str) + __builtin_strlen(Str) #else - std::char_traits::length(Str) + std::char_traits::length(Str) #endif - : 0) { + : 0) { } /// Construct a string ref from a pointer and length. /*implicit*/ constexpr StringRef(const char *data, size_t length) - : Data(data), Length(length) {} + : View(data, length) {} /// Construct a string ref from an std::string. - /*implicit*/ StringRef(const std::string &Str) - : Data(Str.data()), Length(Str.length()) {} + /*implicit*/ StringRef(const std::string &Str) : View(Str) {} /// Construct a string ref from an std::string_view. - /*implicit*/ constexpr StringRef(std::string_view Str) - : Data(Str.data()), Length(Str.size()) {} + /*implicit*/ constexpr StringRef(std::string_view Str) : View(Str) {} /// @} /// @name Iterators @@ -140,13 +134,13 @@ namespace llvm { /// data - Get a pointer to the start of the string (which may not be null /// terminated). - [[nodiscard]] constexpr const char *data() const { return Data; } + [[nodiscard]] constexpr const char *data() const { return View.data(); } /// empty - Check if the string is empty. [[nodiscard]] constexpr bool empty() const { return size() == 0; } /// size - Get the string size. - [[nodiscard]] constexpr size_t size() const { return Length; } + [[nodiscard]] constexpr size_t size() const { return View.size(); } /// front - Get the first character in the string. [[nodiscard]] char front() const { From 242c77018f669c0b8f06b262050fcc4dde486738 Mon Sep 17 00:00:00 2001 From: Kazu Hirata Date: Sat, 26 Oct 2024 19:29:49 -0700 Subject: [PATCH 2/9] [ARM] clang-format (NFC) I'm planning to post a patch in this area. --- .../lib/Target/ARM/AsmParser/ARMAsmParser.cpp | 34 +++++++++---------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp b/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp index 906519fef45db4..68f1199fd12e14 100644 --- a/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp +++ b/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp @@ -5081,23 +5081,23 @@ ParseStatus ARMAsmParser::parseMemBarrierOptOperand(OperandVector &Operands) { StringRef OptStr = Tok.getString(); Opt = StringSwitch(OptStr.slice(0, OptStr.size()).lower()) - .Case("sy", ARM_MB::SY) - .Case("st", ARM_MB::ST) - .Case("ld", ARM_MB::LD) - .Case("sh", ARM_MB::ISH) - .Case("ish", ARM_MB::ISH) - .Case("shst", ARM_MB::ISHST) - .Case("ishst", ARM_MB::ISHST) - .Case("ishld", ARM_MB::ISHLD) - .Case("nsh", ARM_MB::NSH) - .Case("un", ARM_MB::NSH) - .Case("nshst", ARM_MB::NSHST) - .Case("nshld", ARM_MB::NSHLD) - .Case("unst", ARM_MB::NSHST) - .Case("osh", ARM_MB::OSH) - .Case("oshst", ARM_MB::OSHST) - .Case("oshld", ARM_MB::OSHLD) - .Default(~0U); + .Case("sy", ARM_MB::SY) + .Case("st", ARM_MB::ST) + .Case("ld", ARM_MB::LD) + .Case("sh", ARM_MB::ISH) + .Case("ish", ARM_MB::ISH) + .Case("shst", ARM_MB::ISHST) + .Case("ishst", ARM_MB::ISHST) + .Case("ishld", ARM_MB::ISHLD) + .Case("nsh", ARM_MB::NSH) + .Case("un", ARM_MB::NSH) + .Case("nshst", ARM_MB::NSHST) + .Case("nshld", ARM_MB::NSHLD) + .Case("unst", ARM_MB::NSHST) + .Case("osh", ARM_MB::OSH) + .Case("oshst", ARM_MB::OSHST) + .Case("oshld", ARM_MB::OSHLD) + .Default(~0U); // ishld, oshld, nshld and ld are only available from ARMv8. if (!hasV8Ops() && (Opt == ARM_MB::ISHLD || Opt == ARM_MB::OSHLD || From 0dd9fdcf83cd00f51669b32c96937a97ef4b339e Mon Sep 17 00:00:00 2001 From: Kyungwoo Lee Date: Sat, 26 Oct 2024 20:02:05 -0700 Subject: [PATCH 3/9] [StructuralHash] Support Differences (#112638) This computes a structural hash while allowing for selective ignoring of certain operands based on a custom function that is provided. Instead of a single hash value, it now returns FunctionHashInfo which includes a hash value, an instruction mapping, and a map to track the operand location and its corresponding hash value that is ignored. Depends on https://github.com/llvm/llvm-project/pull/112621. This is a patch for https://discourse.llvm.org/t/rfc-global-function-merging/82608. --- llvm/include/llvm/Analysis/StructuralHash.h | 13 +- llvm/include/llvm/IR/StructuralHash.h | 45 ++++++ llvm/lib/Analysis/StructuralHash.cpp | 27 +++- llvm/lib/IR/StructuralHash.cpp | 153 +++++++++++++++--- llvm/lib/Passes/PassBuilder.cpp | 14 +- llvm/lib/Passes/PassRegistry.def | 7 +- .../StructuralHash/structural-hash-printer.ll | 24 ++- llvm/unittests/IR/StructuralHashTest.cpp | 61 +++++++ 8 files changed, 304 insertions(+), 40 deletions(-) diff --git a/llvm/include/llvm/Analysis/StructuralHash.h b/llvm/include/llvm/Analysis/StructuralHash.h index 9f33c69aed345c..4c9f063bc7d2c8 100644 --- a/llvm/include/llvm/Analysis/StructuralHash.h +++ b/llvm/include/llvm/Analysis/StructuralHash.h @@ -13,15 +13,22 @@ namespace llvm { +enum class StructuralHashOptions { + None, /// Hash with opcode only. + Detailed, /// Hash with opcode and operands. + CallTargetIgnored, /// Ignore call target operand when computing hash. +}; + /// Printer pass for StructuralHashes class StructuralHashPrinterPass : public PassInfoMixin { raw_ostream &OS; - bool EnableDetailedStructuralHash; + const StructuralHashOptions Options; public: - explicit StructuralHashPrinterPass(raw_ostream &OS, bool Detailed) - : OS(OS), EnableDetailedStructuralHash(Detailed) {} + explicit StructuralHashPrinterPass(raw_ostream &OS, + StructuralHashOptions Options) + : OS(OS), Options(Options) {} PreservedAnalyses run(Module &M, ModuleAnalysisManager &MAM); diff --git a/llvm/include/llvm/IR/StructuralHash.h b/llvm/include/llvm/IR/StructuralHash.h index e2e192cc9501b3..071575137ff572 100644 --- a/llvm/include/llvm/IR/StructuralHash.h +++ b/llvm/include/llvm/IR/StructuralHash.h @@ -14,7 +14,9 @@ #ifndef LLVM_IR_STRUCTURALHASH_H #define LLVM_IR_STRUCTURALHASH_H +#include "llvm/ADT/MapVector.h" #include "llvm/ADT/StableHashing.h" +#include "llvm/IR/Instruction.h" #include namespace llvm { @@ -35,6 +37,49 @@ stable_hash StructuralHash(const Function &F, bool DetailedHash = false); /// composed the module hash. stable_hash StructuralHash(const Module &M, bool DetailedHash = false); +/// The pair of an instruction index and a operand index. +using IndexPair = std::pair; + +/// A map from an instruction index to an instruction pointer. +using IndexInstrMap = MapVector; + +/// A map from an IndexPair to a stable hash. +using IndexOperandHashMapType = DenseMap; + +/// A function that takes an instruction and an operand index and returns true +/// if the operand should be ignored in the function hash computation. +using IgnoreOperandFunc = std::function; + +struct FunctionHashInfo { + /// A hash value representing the structural content of the function + stable_hash FunctionHash; + /// A mapping from instruction indices to instruction pointers + std::unique_ptr IndexInstruction; + /// A mapping from pairs of instruction indices and operand indices + /// to the hashes of the operands. This can be used to analyze or + /// reconstruct the differences in ignored operands + std::unique_ptr IndexOperandHashMap; + + FunctionHashInfo(stable_hash FuntionHash, + std::unique_ptr IndexInstruction, + std::unique_ptr IndexOperandHashMap) + : FunctionHash(FuntionHash), + IndexInstruction(std::move(IndexInstruction)), + IndexOperandHashMap(std::move(IndexOperandHashMap)) {} +}; + +/// Computes a structural hash of a given function, considering the structure +/// and content of the function's instructions while allowing for selective +/// ignoring of certain operands based on custom criteria. This hash can be used +/// to identify functions that are structurally similar or identical, which is +/// useful in optimizations, deduplication, or analysis tasks. +/// \param F The function to hash. +/// \param IgnoreOp A callable that takes an instruction and an operand index, +/// and returns true if the operand should be ignored in the hash computation. +/// \return A FunctionHashInfo structure +FunctionHashInfo StructuralHashWithDifferences(const Function &F, + IgnoreOperandFunc IgnoreOp); + } // end namespace llvm #endif diff --git a/llvm/lib/Analysis/StructuralHash.cpp b/llvm/lib/Analysis/StructuralHash.cpp index 3a2341fe59ad9c..4f2e003148b606 100644 --- a/llvm/lib/Analysis/StructuralHash.cpp +++ b/llvm/lib/Analysis/StructuralHash.cpp @@ -21,14 +21,33 @@ using namespace llvm; PreservedAnalyses StructuralHashPrinterPass::run(Module &M, ModuleAnalysisManager &MAM) { OS << "Module Hash: " - << format("%016" PRIx64, StructuralHash(M, EnableDetailedStructuralHash)) + << format("%016" PRIx64, + StructuralHash(M, Options != StructuralHashOptions::None)) << "\n"; for (Function &F : M) { if (F.isDeclaration()) continue; - OS << "Function " << F.getName() << " Hash: " - << format("%016" PRIx64, StructuralHash(F, EnableDetailedStructuralHash)) - << "\n"; + if (Options == StructuralHashOptions::CallTargetIgnored) { + auto IgnoreOp = [&](const Instruction *I, unsigned OpndIdx) { + return I->getOpcode() == Instruction::Call && + isa(I->getOperand(OpndIdx)); + }; + auto FuncHashInfo = StructuralHashWithDifferences(F, IgnoreOp); + OS << "Function " << F.getName() + << " Hash: " << format("%016" PRIx64, FuncHashInfo.FunctionHash) + << "\n"; + for (auto &[IndexPair, OpndHash] : *FuncHashInfo.IndexOperandHashMap) { + auto [InstIndex, OpndIndex] = IndexPair; + OS << "\tIgnored Operand Hash: " << format("%016" PRIx64, OpndHash) + << " at (" << InstIndex << "," << OpndIndex << ")\n"; + } + } else { + OS << "Function " << F.getName() << " Hash: " + << format( + "%016" PRIx64, + StructuralHash(F, Options == StructuralHashOptions::Detailed)) + << "\n"; + } } return PreservedAnalyses::all(); } diff --git a/llvm/lib/IR/StructuralHash.cpp b/llvm/lib/IR/StructuralHash.cpp index 267a085c5af705..a51f9124af04da 100644 --- a/llvm/lib/IR/StructuralHash.cpp +++ b/llvm/lib/IR/StructuralHash.cpp @@ -34,14 +34,18 @@ class StructuralHashImpl { static constexpr stable_hash FunctionHeaderHash = 0x62642d6b6b2d6b72; static constexpr stable_hash GlobalHeaderHash = 23456; - // This will produce different values on 32-bit and 64-bit systens as - // hash_combine returns a size_t. However, this is only used for - // detailed hashing which, in-tree, only needs to distinguish between - // differences in functions. - // TODO: This is not stable. - template stable_hash hashArbitaryType(const T &V) { - return hash_combine(V); - } + /// IgnoreOp is a function that returns true if the operand should be ignored. + IgnoreOperandFunc IgnoreOp = nullptr; + /// A mapping from instruction indices to instruction pointers. + /// The index represents the position of an instruction based on the order in + /// which it is first encountered. + std::unique_ptr IndexInstruction = nullptr; + /// A mapping from pairs of instruction indices and operand indices + /// to the hashes of the operands. + std::unique_ptr IndexOperandHashMap = nullptr; + + /// Assign a unique ID to each Value in the order they are first seen. + DenseMap ValueToId; stable_hash hashType(Type *ValueType) { SmallVector Hashes; @@ -53,23 +57,95 @@ class StructuralHashImpl { public: StructuralHashImpl() = delete; - explicit StructuralHashImpl(bool DetailedHash) : DetailedHash(DetailedHash) {} + explicit StructuralHashImpl(bool DetailedHash, + IgnoreOperandFunc IgnoreOp = nullptr) + : DetailedHash(DetailedHash), IgnoreOp(IgnoreOp) { + if (IgnoreOp) { + IndexInstruction = std::make_unique(); + IndexOperandHashMap = std::make_unique(); + } + } + + stable_hash hashAPInt(const APInt &I) { + SmallVector Hashes; + Hashes.emplace_back(I.getBitWidth()); + auto RawVals = ArrayRef(I.getRawData(), I.getNumWords()); + Hashes.append(RawVals.begin(), RawVals.end()); + return stable_hash_combine(Hashes); + } + + stable_hash hashAPFloat(const APFloat &F) { + return hashAPInt(F.bitcastToAPInt()); + } + + stable_hash hashGlobalValue(const GlobalValue *GV) { + if (!GV->hasName()) + return 0; + return stable_hash_name(GV->getName()); + } + // Compute a hash for a Constant. This function is logically similar to + // FunctionComparator::cmpConstants() in FunctionComparator.cpp, but here + // we're interested in computing a hash rather than comparing two Constants. + // Some of the logic is simplified, e.g, we don't expand GEPOperator. stable_hash hashConstant(Constant *C) { SmallVector Hashes; - // TODO: hashArbitaryType() is not stable. - if (ConstantInt *ConstInt = dyn_cast(C)) { - Hashes.emplace_back(hashArbitaryType(ConstInt->getValue())); - } else if (ConstantFP *ConstFP = dyn_cast(C)) { - Hashes.emplace_back(hashArbitaryType(ConstFP->getValue())); - } else if (Function *Func = dyn_cast(C)) { - // Hashing the name will be deterministic as LLVM's hashing infrastructure - // has explicit support for hashing strings and will not simply hash - // the pointer. - Hashes.emplace_back(hashArbitaryType(Func->getName())); + + Type *Ty = C->getType(); + Hashes.emplace_back(hashType(Ty)); + + if (C->isNullValue()) { + Hashes.emplace_back(static_cast('N')); + return stable_hash_combine(Hashes); } - return stable_hash_combine(Hashes); + if (auto *G = dyn_cast(C)) { + Hashes.emplace_back(hashGlobalValue(G)); + return stable_hash_combine(Hashes); + } + + if (const auto *Seq = dyn_cast(C)) { + Hashes.emplace_back(xxh3_64bits(Seq->getRawDataValues())); + return stable_hash_combine(Hashes); + } + + switch (C->getValueID()) { + case Value::ConstantIntVal: { + const APInt &Int = cast(C)->getValue(); + Hashes.emplace_back(hashAPInt(Int)); + return stable_hash_combine(Hashes); + } + case Value::ConstantFPVal: { + const APFloat &APF = cast(C)->getValueAPF(); + Hashes.emplace_back(hashAPFloat(APF)); + return stable_hash_combine(Hashes); + } + case Value::ConstantArrayVal: + case Value::ConstantStructVal: + case Value::ConstantVectorVal: + case Value::ConstantExprVal: { + for (const auto &Op : C->operands()) { + auto H = hashConstant(cast(Op)); + Hashes.emplace_back(H); + } + return stable_hash_combine(Hashes); + } + case Value::BlockAddressVal: { + const BlockAddress *BA = cast(C); + auto H = hashGlobalValue(BA->getFunction()); + Hashes.emplace_back(H); + return stable_hash_combine(Hashes); + } + case Value::DSOLocalEquivalentVal: { + const auto *Equiv = cast(C); + auto H = hashGlobalValue(Equiv->getGlobalValue()); + Hashes.emplace_back(H); + return stable_hash_combine(Hashes); + } + default: + // Skip other types of constants for simplicity. + return stable_hash_combine(Hashes); + } } stable_hash hashValue(Value *V) { @@ -83,6 +159,10 @@ class StructuralHashImpl { if (Argument *Arg = dyn_cast(V)) Hashes.emplace_back(Arg->getArgNo()); + // Get an index (an insertion order) for the non-constant value. + auto [It, WasInserted] = ValueToId.try_emplace(V, ValueToId.size()); + Hashes.emplace_back(It->second); + return stable_hash_combine(Hashes); } @@ -107,8 +187,20 @@ class StructuralHashImpl { if (const auto *ComparisonInstruction = dyn_cast(&Inst)) Hashes.emplace_back(ComparisonInstruction->getPredicate()); - for (const auto &Op : Inst.operands()) - Hashes.emplace_back(hashOperand(Op)); + unsigned InstIdx = 0; + if (IndexInstruction) { + InstIdx = IndexInstruction->size(); + IndexInstruction->try_emplace(InstIdx, const_cast(&Inst)); + } + + for (const auto [OpndIdx, Op] : enumerate(Inst.operands())) { + auto OpndHash = hashOperand(Op); + if (IgnoreOp && IgnoreOp(&Inst, OpndIdx)) { + assert(IndexOperandHashMap); + IndexOperandHashMap->try_emplace({InstIdx, OpndIdx}, OpndHash); + } else + Hashes.emplace_back(OpndHash); + } return stable_hash_combine(Hashes); } @@ -188,6 +280,14 @@ class StructuralHashImpl { } uint64_t getHash() const { return Hash; } + + std::unique_ptr getIndexInstrMap() { + return std::move(IndexInstruction); + } + + std::unique_ptr getIndexPairOpndHashMap() { + return std::move(IndexOperandHashMap); + } }; } // namespace @@ -203,3 +303,12 @@ stable_hash llvm::StructuralHash(const Module &M, bool DetailedHash) { H.update(M); return H.getHash(); } + +FunctionHashInfo +llvm::StructuralHashWithDifferences(const Function &F, + IgnoreOperandFunc IgnoreOp) { + StructuralHashImpl H(/*DetailedHash=*/true, IgnoreOp); + H.update(F); + return FunctionHashInfo(H.getHash(), H.getIndexInstrMap(), + H.getIndexPairOpndHashMap()); +} diff --git a/llvm/lib/Passes/PassBuilder.cpp b/llvm/lib/Passes/PassBuilder.cpp index f5ce405ab8d961..d1f75dfb5350a0 100644 --- a/llvm/lib/Passes/PassBuilder.cpp +++ b/llvm/lib/Passes/PassBuilder.cpp @@ -1175,9 +1175,17 @@ Expected parseMemProfUsePassOptions(StringRef Params) { return Result; } -Expected parseStructuralHashPrinterPassOptions(StringRef Params) { - return PassBuilder::parseSinglePassOption(Params, "detailed", - "StructuralHashPrinterPass"); +Expected +parseStructuralHashPrinterPassOptions(StringRef Params) { + if (Params.empty()) + return StructuralHashOptions::None; + if (Params == "detailed") + return StructuralHashOptions::Detailed; + if (Params == "call-target-ignored") + return StructuralHashOptions::CallTargetIgnored; + return make_error( + formatv("invalid structural hash printer parameter '{0}' ", Params).str(), + inconvertibleErrorCode()); } Expected parseWinEHPrepareOptions(StringRef Params) { diff --git a/llvm/lib/Passes/PassRegistry.def b/llvm/lib/Passes/PassRegistry.def index 549c1359b5852c..017ae311c55eb4 100644 --- a/llvm/lib/Passes/PassRegistry.def +++ b/llvm/lib/Passes/PassRegistry.def @@ -220,10 +220,11 @@ MODULE_PASS_WITH_PARAMS( parseMSanPassOptions, "recover;kernel;eager-checks;track-origins=N") MODULE_PASS_WITH_PARAMS( "print", "StructuralHashPrinterPass", - [](bool EnableDetailedStructuralHash) { - return StructuralHashPrinterPass(dbgs(), EnableDetailedStructuralHash); + [](StructuralHashOptions Options) { + return StructuralHashPrinterPass(dbgs(), Options); }, - parseStructuralHashPrinterPassOptions, "detailed") + parseStructuralHashPrinterPassOptions, "detailed;call-target-ignored") + #undef MODULE_PASS_WITH_PARAMS #ifndef CGSCC_ANALYSIS diff --git a/llvm/test/Analysis/StructuralHash/structural-hash-printer.ll b/llvm/test/Analysis/StructuralHash/structural-hash-printer.ll index 5936199bf32f43..3c23b54d297369 100644 --- a/llvm/test/Analysis/StructuralHash/structural-hash-printer.ll +++ b/llvm/test/Analysis/StructuralHash/structural-hash-printer.ll @@ -1,17 +1,21 @@ ; RUN: opt -passes='print' -disable-output %s 2>&1 | FileCheck %s ; RUN: opt -passes='print' -disable-output %s 2>&1 | FileCheck %s -check-prefix=DETAILED-HASH +; RUN: opt -passes='print' -disable-output %s 2>&1 | FileCheck %s -check-prefix=CALLTARGETIGNORED-HASH ; Add a declaration so that we can test we skip it. -declare i64 @d1() +declare i64 @d1(i64) +declare i64 @e1(i64) define i64 @f1(i64 %a) { %b = add i64 %a, 1 - ret i64 %b + %c = call i64 @d1(i64 %b) + ret i64 %c } -define i32 @f2(i32 %a) { - %b = add i32 %a, 2 - ret i32 %b +define i64 @f2(i64 %a) { + %b = add i64 %a, 1 + %c = call i64 @e1(i64 %b) + ret i64 %c } ; CHECK: Module Hash: {{([a-f0-9]{16,})}} @@ -22,3 +26,13 @@ define i32 @f2(i32 %a) { ; DETAILED-HASH-NEXT: Function f1 Hash: [[DF1H:([a-f0-9]{16,})]] ; DETAILED-HASH-NOT: [[DF1H]] ; DETAILED-HASH-NEXT: Function f2 Hash: {{([a-f0-9]{16,})}} + +; When ignoring the call target, check if `f1` and `f2` produce the same function hash. +; The index for the call instruction is 1, and the index of the call target operand is 1. +; The ignored operand hashes for different call targets should be different. +; CALLTARGETIGNORED-HASH: Module Hash: {{([a-f0-9]{16,})}} +; CALLTARGETIGNORED-HASH-NEXT: Function f1 Hash: [[IF1H:([a-f0-9]{16,})]] +; CALLTARGETIGNORED-HASH-NEXT: Ignored Operand Hash: [[IO1H:([a-f0-9]{16,})]] at (1,1) +; CALLTARGETIGNORED-HASH-NEXT: Function f2 Hash: [[IF1H]] +; CALLTARGETIGNORED-HASH-NOT: [[IO1H]] +; CALLTARGETIGNORED-HASH-NEXT: Ignored Operand Hash: {{([a-f0-9]{16,})}} at (1,1) diff --git a/llvm/unittests/IR/StructuralHashTest.cpp b/llvm/unittests/IR/StructuralHashTest.cpp index 64e66aa5f97a6d..81c17120a1f6ff 100644 --- a/llvm/unittests/IR/StructuralHashTest.cpp +++ b/llvm/unittests/IR/StructuralHashTest.cpp @@ -10,6 +10,7 @@ #include "llvm/AsmParser/Parser.h" #include "llvm/IR/Module.h" #include "llvm/Support/SourceMgr.h" +#include "gmock/gmock-matchers.h" #include "gtest/gtest.h" #include @@ -18,6 +19,11 @@ using namespace llvm; namespace { +using testing::Contains; +using testing::Key; +using testing::Pair; +using testing::SizeIs; + std::unique_ptr parseIR(LLVMContext &Context, const char *IR) { SMDiagnostic Err; std::unique_ptr M = parseAssemblyString(IR, Err, Context); @@ -239,4 +245,59 @@ TEST(StructuralHashTest, ArgumentNumber) { EXPECT_EQ(StructuralHash(*M1), StructuralHash(*M2)); EXPECT_NE(StructuralHash(*M1, true), StructuralHash(*M2, true)); } + +TEST(StructuralHashTest, Differences) { + LLVMContext Ctx; + std::unique_ptr M1 = parseIR(Ctx, "define i64 @f(i64 %a) {\n" + " %c = add i64 %a, 1\n" + " %b = call i64 @f1(i64 %c)\n" + " ret i64 %b\n" + "}\n" + "declare i64 @f1(i64)"); + auto *F1 = M1->getFunction("f"); + std::unique_ptr M2 = parseIR(Ctx, "define i64 @g(i64 %a) {\n" + " %c = add i64 %a, 1\n" + " %b = call i64 @f2(i64 %c)\n" + " ret i64 %b\n" + "}\n" + "declare i64 @f2(i64)"); + auto *F2 = M2->getFunction("g"); + + // They are originally different when not ignoring any operand. + EXPECT_NE(StructuralHash(*F1, true), StructuralHash(*F2, true)); + EXPECT_NE(StructuralHashWithDifferences(*F1, nullptr).FunctionHash, + StructuralHashWithDifferences(*F2, nullptr).FunctionHash); + + // When we ignore the call target f1 vs f2, they have the same hash. + auto IgnoreOp = [&](const Instruction *I, unsigned OpndIdx) { + return I->getOpcode() == Instruction::Call && OpndIdx == 1; + }; + auto FuncHashInfo1 = StructuralHashWithDifferences(*F1, IgnoreOp); + auto FuncHashInfo2 = StructuralHashWithDifferences(*F2, IgnoreOp); + EXPECT_EQ(FuncHashInfo1.FunctionHash, FuncHashInfo2.FunctionHash); + + // There are total 3 instructions. + EXPECT_THAT(*FuncHashInfo1.IndexInstruction, SizeIs(3)); + EXPECT_THAT(*FuncHashInfo2.IndexInstruction, SizeIs(3)); + + // The only 1 operand (the call target) has been ignored. + EXPECT_THAT(*FuncHashInfo1.IndexOperandHashMap, SizeIs(1u)); + EXPECT_THAT(*FuncHashInfo2.IndexOperandHashMap, SizeIs(1u)); + + // The index pair of instruction and operand (1, 1) is a key in the map. + ASSERT_THAT(*FuncHashInfo1.IndexOperandHashMap, Contains(Key(Pair(1, 1)))); + ASSERT_THAT(*FuncHashInfo2.IndexOperandHashMap, Contains(Key(Pair(1, 1)))); + + // The indexed instruciton must be the call instruction as shown in the + // IgnoreOp above. + EXPECT_EQ(FuncHashInfo1.IndexInstruction->lookup(1)->getOpcode(), + Instruction::Call); + EXPECT_EQ(FuncHashInfo2.IndexInstruction->lookup(1)->getOpcode(), + Instruction::Call); + + // The ignored operand hashes (for f1 vs. f2) are different. + EXPECT_NE(FuncHashInfo1.IndexOperandHashMap->lookup({1, 1}), + FuncHashInfo2.IndexOperandHashMap->lookup({1, 1})); +} + } // end anonymous namespace From 93da6423af5f00a3bbee4d2ee571ccc7887f444d Mon Sep 17 00:00:00 2001 From: Sirui Mu Date: Sun, 27 Oct 2024 11:52:00 +0800 Subject: [PATCH 4/9] [mlir][LLVM] Add builders for llvm.intr.assume (#113317) This patch adds several new builders for llvm.intr.assume that build the operation with additional operand bundles. --- .../include/mlir/Dialect/LLVMIR/LLVMDialect.h | 8 ++++ .../mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td | 9 ++++- mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp | 39 ++++++++++++++++++- 3 files changed, 54 insertions(+), 2 deletions(-) diff --git a/mlir/include/mlir/Dialect/LLVMIR/LLVMDialect.h b/mlir/include/mlir/Dialect/LLVMIR/LLVMDialect.h index d236cae0d80882..63e007cdc335cc 100644 --- a/mlir/include/mlir/Dialect/LLVMIR/LLVMDialect.h +++ b/mlir/include/mlir/Dialect/LLVMIR/LLVMDialect.h @@ -33,6 +33,7 @@ #include "mlir/Support/ThreadLocalCache.h" #include "llvm/ADT/PointerEmbeddedInt.h" #include "llvm/IR/DerivedTypes.h" +#include "llvm/IR/InstrTypes.h" #include "llvm/IR/LLVMContext.h" #include "llvm/IR/Module.h" #include "llvm/IR/Type.h" @@ -87,6 +88,13 @@ class GEPArg : public PointerUnion { } // namespace LLVM } // namespace mlir +namespace mlir { +namespace LLVM { +struct AssumeAlignTag {}; +struct AssumeSeparateStorageTag {}; +} // namespace LLVM +} // namespace mlir + ///// Ops ///// #define GET_OP_CLASSES #include "mlir/Dialect/LLVMIR/LLVMOps.h.inc" diff --git a/mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td b/mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td index 845c88b1be7750..d07ebbacc60434 100644 --- a/mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td +++ b/mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td @@ -450,7 +450,14 @@ def LLVM_AssumeOp }]; let builders = [ - OpBuilder<(ins "Value":$cond)> + OpBuilder<(ins "Value":$cond)>, + OpBuilder<(ins "Value":$cond, + "ArrayRef>":$opBundles)>, + OpBuilder<(ins "Value":$cond, "llvm::StringRef":$tag, "ValueRange":$args)>, + OpBuilder<(ins "Value":$cond, "AssumeAlignTag":$tag, "Value":$ptr, + "Value":$align)>, + OpBuilder<(ins "Value":$cond, "AssumeSeparateStorageTag":$tag, + "Value":$ptr1, "Value":$ptr2)> ]; let hasVerifier = 1; diff --git a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp index cc73878a64ff67..c9bc9533ca2a6b 100644 --- a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp +++ b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp @@ -3438,7 +3438,44 @@ void InlineAsmOp::getEffects( void LLVM::AssumeOp::build(OpBuilder &builder, OperationState &state, mlir::Value cond) { return build(builder, state, cond, /*op_bundle_operands=*/{}, - /*op_bundle_tags=*/{}); + /*op_bundle_tags=*/ArrayAttr{}); +} + +void LLVM::AssumeOp::build(OpBuilder &builder, OperationState &state, + Value cond, + ArrayRef> opBundles) { + SmallVector opBundleOperands; + SmallVector opBundleTags; + opBundleOperands.reserve(opBundles.size()); + opBundleTags.reserve(opBundles.size()); + + for (const llvm::OperandBundleDefT &bundle : opBundles) { + opBundleOperands.emplace_back(bundle.inputs()); + opBundleTags.push_back( + StringAttr::get(builder.getContext(), bundle.getTag())); + } + + auto opBundleTagsAttr = ArrayAttr::get(builder.getContext(), opBundleTags); + return build(builder, state, cond, opBundleOperands, opBundleTagsAttr); +} + +void LLVM::AssumeOp::build(OpBuilder &builder, OperationState &state, + Value cond, llvm::StringRef tag, ValueRange args) { + llvm::OperandBundleDefT opBundle( + tag.str(), SmallVector(args.begin(), args.end())); + return build(builder, state, cond, opBundle); +} + +void LLVM::AssumeOp::build(OpBuilder &builder, OperationState &state, + Value cond, AssumeAlignTag, Value ptr, Value align) { + return build(builder, state, cond, "align", ValueRange{ptr, align}); +} + +void LLVM::AssumeOp::build(OpBuilder &builder, OperationState &state, + Value cond, AssumeSeparateStorageTag, Value ptr1, + Value ptr2) { + return build(builder, state, cond, "separate_storage", + ValueRange{ptr1, ptr2}); } LogicalResult LLVM::AssumeOp::verify() { return verifyOperandBundles(*this); } From 355e6948d44a97781cc184a22c9b51760cae6de0 Mon Sep 17 00:00:00 2001 From: Teresa Johnson Date: Sat, 26 Oct 2024 20:53:20 -0700 Subject: [PATCH 5/9] [MemProf] Fix clone edge comparison (#113753) The issue fixed in PR113337 exposed a bug in the comparisons done in allocTypesMatch, which compares a vector of alloc types to those in the given vector of Edges. The form of std::equal used, which didn't provide the end iterator for the Edges vector, will iterate through as many entries in the Edges vector as in the InAllocTypes vector, which can fail if there are fewer entries in the Edges vector, because we may dereference a bogus Edge pointer. This function is called twice, once for the Node, with its callee edges, in which case the number of edges should always match the number of entries in allocTypesMatch, which is computed from the Node's callee edges. It was also called for Node's clones, and it turns out that after cloning and edge modifications done for other allocations, the number of callee edges in Node and its clones may no longer match. In some cases, more common with memprof ICP before the PR113337, the number of clone edges can be smaller leading to a bad dereference. I found for a large application even before adding memprof ICP support we sometimes call this with fewer entries in the clone's callee edges, but were getting lucky as they had allocation type None, and we didn't end up attempting to dereference the bad edge pointer. Fix this by passing Edges.end() to std::equal, which means std::equal will fail if the number of entries in the 2 vectors are not equal. However, this is too conservative, as clone edges may have been added or removed since it was initially cloned, and in fact can be wrong as we may not be comparing allocation types corresponding to the same callee. Therefore, a couple of enhancements are made to avoid regressing and improve the checking and cloning: - Don't bother calling the alloc type comparison when the clone and the Node's alloc type for the current allocation are precise (have a single allocation type) and are the same (which is guaranteed by an earlier check, and an assert is added to confirm that). In that case we can trivially determine that the clone can be used. - Split the alloc type matching handling into a separate function for the clone case. In that case, for each of the InAllocType entries, attempt to find and compare to the clone callee edge with the same callee as the corresponding original node callee. To create a test case I needed to take a spec application (xalancbmk), and repeatedly apply random hot/cold-ness to the memprof contexts when building, until I hit the problematic case. I then reduced that full LTO IR using llvm-reduce and then manually. --- .../IPO/MemProfContextDisambiguation.cpp | 66 +++++++++++-- .../fix_clone_checking.ll | 99 +++++++++++++++++++ 2 files changed, 159 insertions(+), 6 deletions(-) create mode 100644 llvm/test/Transforms/MemProfContextDisambiguation/fix_clone_checking.ll diff --git a/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp b/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp index 905186edcbecc4..da5ded23ecc045 100644 --- a/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp +++ b/llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp @@ -928,8 +928,11 @@ bool allocTypesMatch( const std::vector &InAllocTypes, const std::vector>> &Edges) { + // This should be called only when the InAllocTypes vector was computed for + // this set of Edges. Make sure the sizes are the same. + assert(InAllocTypes.size() == Edges.size()); return std::equal( - InAllocTypes.begin(), InAllocTypes.end(), Edges.begin(), + InAllocTypes.begin(), InAllocTypes.end(), Edges.begin(), Edges.end(), [](const uint8_t &l, const std::shared_ptr> &r) { // Can share if one of the edges is None type - don't @@ -942,6 +945,46 @@ bool allocTypesMatch( }); } +// Helper to check if the alloc types for all edges recorded in the +// InAllocTypes vector match the alloc types for callee edges in the given +// clone. Because the InAllocTypes were computed from the original node's callee +// edges, and other cloning could have happened after this clone was created, we +// need to find the matching clone callee edge, which may or may not exist. +template +bool allocTypesMatchClone( + const std::vector &InAllocTypes, + const ContextNode *Clone) { + const ContextNode *Node = Clone->CloneOf; + assert(Node); + // InAllocTypes should have been computed for the original node's callee + // edges. + assert(InAllocTypes.size() == Node->CalleeEdges.size()); + // First create a map of the clone callee edge callees to the edge alloc type. + DenseMap *, uint8_t> + EdgeCalleeMap; + for (const auto &E : Clone->CalleeEdges) { + assert(!EdgeCalleeMap.contains(E->Callee)); + EdgeCalleeMap[E->Callee] = E->AllocTypes; + } + // Next, walk the original node's callees, and look for the corresponding + // clone edge to that callee. + for (unsigned I = 0; I < Node->CalleeEdges.size(); I++) { + auto Iter = EdgeCalleeMap.find(Node->CalleeEdges[I]->Callee); + // Not found is ok, we will simply add an edge if we use this clone. + if (Iter == EdgeCalleeMap.end()) + continue; + // Can share if one of the edges is None type - don't + // care about the type along that edge as it doesn't + // exist for those context ids. + if (InAllocTypes[I] == (uint8_t)AllocationType::None || + Iter->second == (uint8_t)AllocationType::None) + continue; + if (allocTypeToUse(Iter->second) != allocTypeToUse(InAllocTypes[I])) + return false; + } + return true; +} + } // end anonymous namespace template @@ -3364,11 +3407,22 @@ void CallsiteContextGraph::identifyClones( allocTypeToUse(CallerAllocTypeForAlloc)) continue; - if (!allocTypesMatch( - CalleeEdgeAllocTypesForCallerEdge, CurClone->CalleeEdges)) - continue; - Clone = CurClone; - break; + bool BothSingleAlloc = hasSingleAllocType(CurClone->AllocTypes) && + hasSingleAllocType(CallerAllocTypeForAlloc); + // The above check should mean that if both have single alloc types that + // they should be equal. + assert(!BothSingleAlloc || + CurClone->AllocTypes == CallerAllocTypeForAlloc); + + // If either both have a single alloc type (which are the same), or if the + // clone's callee edges have the same alloc types as those for the current + // allocation on Node's callee edges (CalleeEdgeAllocTypesForCallerEdge), + // then we can reuse this clone. + if (BothSingleAlloc || allocTypesMatchClone( + CalleeEdgeAllocTypesForCallerEdge, CurClone)) { + Clone = CurClone; + break; + } } // The edge iterator is adjusted when we move the CallerEdge to the clone. diff --git a/llvm/test/Transforms/MemProfContextDisambiguation/fix_clone_checking.ll b/llvm/test/Transforms/MemProfContextDisambiguation/fix_clone_checking.ll new file mode 100644 index 00000000000000..75cebae0b82971 --- /dev/null +++ b/llvm/test/Transforms/MemProfContextDisambiguation/fix_clone_checking.ll @@ -0,0 +1,99 @@ +;; Test to make sure we don't fail when cloning in a case where we end up with +;; a clone that has fewer edges than the node it was initially cloned from. +;; This test was reduced and simplified from xalancbmk with some random hotness +;; applied to the profile that reproduced the issue. + +; RUN: opt -passes=memprof-context-disambiguation -supports-hot-cold-new \ +; RUN: -memprof-verify-ccg -memprof-verify-nodes \ +; RUN: -pass-remarks=memprof-context-disambiguation %s -S 2>&1 | FileCheck %s + +;; Make sure we created some clones +; CHECK: created clone A.memprof.1 +; CHECK: created clone C.memprof.1 +; CHECK: created clone D.memprof.1 +; CHECK: created clone E.memprof.1 +; CHECK: created clone B.memprof.1 +; CHECK: created clone F.memprof.1 +; CHECK: created clone G.memprof.1 + +; ModuleID = '' +source_filename = "reduced.ll" +target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128" +target triple = "x86_64-grtev4-linux-gnu" + +define void @A() { + call void @B(), !callsite !0 + ret void +} + +define void @C() { + call void @D(), !callsite !1 + ret void +} + +define void @D() { + call void @A(), !callsite !2 + ret void +} + +define void @E() { + %1 = call ptr @_Znwm(i64 0), !memprof !3, !callsite !20 + ret void +} + +define void @B() { + call void @F(), !callsite !21 + ret void +} + +define void @F() { + call void @E(), !callsite !22 + call void @G(), !callsite !23 + ret void +} + +define void @G() { + %1 = call ptr @_Znwm(i64 0), !memprof !24, !callsite !37 + ret void +} + +declare ptr @_Znwm(i64) + +!0 = !{i64 1995602625719775354} +!1 = !{i64 4312698517630782220} +!2 = !{i64 5516454029445989383} +!3 = !{!4, !6, !8, !10, !12, !14, !16, !18} +!4 = !{!5, !"notcold"} +!5 = !{i64 -2282665745786859978, i64 -6758300505622211768, i64 2938340307832638862, i64 7147584705143805656, i64 -6456074186195384663, i64 1995602625719775354} +!6 = !{!7, !"cold"} +!7 = !{i64 -2282665745786859978, i64 -6758300505622211768, i64 2938340307832638862, i64 7147584705143805656, i64 -6456074186195384663, i64 2077908580042347045, i64 4312698517630782220, i64 5379466077518675850} +!8 = !{!9, !"cold"} +!9 = !{i64 -2282665745786859978, i64 -6758300505622211768, i64 2938340307832638862, i64 7147584705143805656, i64 -6456074186195384663, i64 2077908580042347045, i64 4312698517630782220, i64 -7632894069000375689} +!10 = !{!11, !"cold"} +!11 = !{i64 -2282665745786859978, i64 -6758300505622211768, i64 2938340307832638862, i64 7147584705143805656, i64 -6456074186195384663, i64 2939944783060497247} +!12 = !{!13, !"notcold"} +!13 = !{i64 -2282665745786859978, i64 -6758300505622211768, i64 2938340307832638862, i64 7147584705143805656, i64 -6456074186195384663, i64 5642549674080861567, i64 5516454029445989383, i64 4312698517630782220, i64 -7632894069000375689} +!14 = !{!15, !"cold"} +!15 = !{i64 -2282665745786859978, i64 -6758300505622211768, i64 2938340307832638862, i64 7147584705143805656, i64 -6456074186195384663, i64 5642549674080861567, i64 5516454029445989383, i64 4312698517630782220, i64 -1805555115991223293} +!16 = !{!17, !"notcold"} +!17 = !{i64 -2282665745786859978, i64 -6758300505622211768, i64 2938340307832638862, i64 7147584705143805656, i64 -6456074186195384663, i64 -4746997736434041076, i64 5516454029445989383, i64 4312698517630782220, i64 -1805555115991223293} +!18 = !{!19, !"notcold"} +!19 = !{i64 -2282665745786859978, i64 -6758300505622211768, i64 2938340307832638862, i64 7147584705143805656, i64 -6456074186195384663, i64 -4637272929643682959} +!20 = !{i64 -2282665745786859978, i64 -6758300505622211768, i64 2938340307832638862} +!21 = !{i64 -6456074186195384663} +!22 = !{i64 7147584705143805656} +!23 = !{i64 3938822378769440754} +!24 = !{!25, !27, !29, !31, !33, !35} +!25 = !{!26, !"cold"} +!26 = !{i64 -2282665745786859978, i64 -3548918226713766361, i64 4077289288013931196, i64 3938822378769440754, i64 -6456074186195384663, i64 1995602625719775354, i64 5516454029445989383, i64 4312698517630782220, i64 -1805555115991223293} +!27 = !{!28, !"notcold"} +!28 = !{i64 -2282665745786859978, i64 -3548918226713766361, i64 4077289288013931196, i64 3938822378769440754, i64 -6456074186195384663, i64 2077908580042347045, i64 4312698517630782220, i64 -7632894069000375689} +!29 = !{!30, !"cold"} +!30 = !{i64 -2282665745786859978, i64 -3548918226713766361, i64 4077289288013931196, i64 3938822378769440754, i64 -6456074186195384663, i64 -4746997736434041076, i64 5516454029445989383, i64 4312698517630782220, i64 -7632894069000375689} +!31 = !{!32, !"notcold"} +!32 = !{i64 -2282665745786859978, i64 -3548918226713766361, i64 4077289288013931196, i64 3938822378769440754, i64 -6456074186195384663, i64 -4746997736434041076, i64 5516454029445989383, i64 4312698517630782220, i64 -1805555115991223293} +!33 = !{!34, !"cold"} +!34 = !{i64 -2282665745786859978, i64 -3548918226713766361, i64 4077289288013931196, i64 3938822378769440754, i64 -6456074186195384663, i64 -4637272929643682959} +!35 = !{!36, !"notcold"} +!36 = !{i64 -2282665745786859978, i64 -3548918226713766361, i64 4077289288013931196, i64 3938822378769440754, i64 -6456074186195384663, i64 -4409412896859835674} +!37 = !{i64 -2282665745786859978, i64 -3548918226713766361, i64 4077289288013931196} From d5b42db00f0b21855501b01e8cd80326e1ce763d Mon Sep 17 00:00:00 2001 From: Timm Baeder Date: Sun, 27 Oct 2024 04:56:53 +0100 Subject: [PATCH 6/9] [clang][bytecode][NFC] Only do CheckConstant checks for global pointers (#113786) We can check isStatic() early here and save ourselves some work. --- clang/lib/AST/ByteCode/Interp.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/lib/AST/ByteCode/Interp.cpp b/clang/lib/AST/ByteCode/Interp.cpp index b7a6c224c80f8e..6d40fb03696d48 100644 --- a/clang/lib/AST/ByteCode/Interp.cpp +++ b/clang/lib/AST/ByteCode/Interp.cpp @@ -400,7 +400,7 @@ bool CheckConstant(InterpState &S, CodePtr OpPC, const Descriptor *Desc) { } static bool CheckConstant(InterpState &S, CodePtr OpPC, const Pointer &Ptr) { - if (!Ptr.isBlockPointer()) + if (!Ptr.isStatic() || !Ptr.isBlockPointer()) return true; return CheckConstant(S, OpPC, Ptr.getDeclDesc()); } From 7b88e7530d4329ff0c7c8638f69b39fa1e540218 Mon Sep 17 00:00:00 2001 From: Timm Baeder Date: Sun, 27 Oct 2024 05:06:47 +0100 Subject: [PATCH 7/9] [clang][bytecode][NFC] Make CheckVolatile static (#113785) --- clang/lib/AST/ByteCode/Interp.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clang/lib/AST/ByteCode/Interp.cpp b/clang/lib/AST/ByteCode/Interp.cpp index 6d40fb03696d48..6e45cfb7e8a20c 100644 --- a/clang/lib/AST/ByteCode/Interp.cpp +++ b/clang/lib/AST/ByteCode/Interp.cpp @@ -513,8 +513,8 @@ bool CheckMutable(InterpState &S, CodePtr OpPC, const Pointer &Ptr) { return false; } -bool CheckVolatile(InterpState &S, CodePtr OpPC, const Pointer &Ptr, - AccessKinds AK) { +static bool CheckVolatile(InterpState &S, CodePtr OpPC, const Pointer &Ptr, + AccessKinds AK) { assert(Ptr.isLive()); // FIXME: This check here might be kinda expensive. Maybe it would be better From 60d2feded5c0f55b21d042ee2f35227847d66ee0 Mon Sep 17 00:00:00 2001 From: Kazu Hirata Date: Sat, 26 Oct 2024 22:07:56 -0700 Subject: [PATCH 8/9] [ARM] Remove a redundant call to StringRef::slice (NFC) (#113783) OptStr.slice(0, OptStr.size()) is exactly the same as OptStr. --- llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp b/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp index 68f1199fd12e14..0df1c336a22146 100644 --- a/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp +++ b/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp @@ -5080,7 +5080,7 @@ ParseStatus ARMAsmParser::parseMemBarrierOptOperand(OperandVector &Operands) { if (Tok.is(AsmToken::Identifier)) { StringRef OptStr = Tok.getString(); - Opt = StringSwitch(OptStr.slice(0, OptStr.size()).lower()) + Opt = StringSwitch(OptStr.lower()) .Case("sy", ARM_MB::SY) .Case("st", ARM_MB::ST) .Case("ld", ARM_MB::LD) From d2e9532fe12dc2568f40c2648ff4bb3730141aed Mon Sep 17 00:00:00 2001 From: Eirik Byrkjeflot Anonsen Date: Sun, 27 Oct 2024 10:09:39 +0100 Subject: [PATCH 9/9] [DemoteRegToStack] Use correct variable for branch instructions in DemoteRegToStack (#113798) I happened to see this code, and it seems "obviously" wrong to me. So here's what I think this code is supposed to look like. --- llvm/lib/Transforms/Utils/DemoteRegToStack.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/llvm/lib/Transforms/Utils/DemoteRegToStack.cpp b/llvm/lib/Transforms/Utils/DemoteRegToStack.cpp index 3a33b591d35582..6337913cdbbeb7 100644 --- a/llvm/lib/Transforms/Utils/DemoteRegToStack.cpp +++ b/llvm/lib/Transforms/Utils/DemoteRegToStack.cpp @@ -55,8 +55,8 @@ AllocaInst *llvm::DemoteRegToStack(Instruction &I, bool VolatileLoads, for (unsigned i = 0; i < CBI->getNumSuccessors(); i++) { auto *Succ = CBI->getSuccessor(i); if (!Succ->getSinglePredecessor()) { - assert(isCriticalEdge(II, i) && "Expected a critical edge!"); - [[maybe_unused]] BasicBlock *BB = SplitCriticalEdge(II, i); + assert(isCriticalEdge(CBI, i) && "Expected a critical edge!"); + [[maybe_unused]] BasicBlock *BB = SplitCriticalEdge(CBI, i); assert(BB && "Unable to split critical edge."); } }