-
Notifications
You must be signed in to change notification settings - Fork 12.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Revert "Fix LLVM_ENABLE_ABI_BREAKING_CHECKS macro check: use #if inst… #110923
Conversation
…ead of #ifdef (llvm#110883)" This reverts commit 1905cdb.
@llvm/pr-subscribers-llvm-selectiondag @llvm/pr-subscribers-mlir Author: Christopher Di Bella (cjdb) Changes…ead of #ifdef (#110883)" This reverts commit 1905cdb. Full diff: https://github.com/llvm/llvm-project/pull/110923.diff 13 Files Affected:
diff --git a/llvm/include/llvm/CodeGen/SelectionDAG.h b/llvm/include/llvm/CodeGen/SelectionDAG.h
index b8f80738486a71..d6c2c36a0d482a 100644
--- a/llvm/include/llvm/CodeGen/SelectionDAG.h
+++ b/llvm/include/llvm/CodeGen/SelectionDAG.h
@@ -584,7 +584,7 @@ class SelectionDAG {
return Root;
}
-#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+#ifndef NDEBUG
void VerifyDAGDivergence();
#endif
diff --git a/llvm/include/llvm/Passes/StandardInstrumentations.h b/llvm/include/llvm/Passes/StandardInstrumentations.h
index 80eedc52bc324e..fa9c744294a666 100644
--- a/llvm/include/llvm/Passes/StandardInstrumentations.h
+++ b/llvm/include/llvm/Passes/StandardInstrumentations.h
@@ -171,7 +171,7 @@ class PreservedCFGCheckerInstrumentation {
FunctionAnalysisManager::Invalidator &);
};
-#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
SmallVector<StringRef, 8> PassStack;
#endif
diff --git a/llvm/include/llvm/Support/GenericDomTreeConstruction.h b/llvm/include/llvm/Support/GenericDomTreeConstruction.h
index 2e21bdc9fce2d5..9aab5ec60f4a25 100644
--- a/llvm/include/llvm/Support/GenericDomTreeConstruction.h
+++ b/llvm/include/llvm/Support/GenericDomTreeConstruction.h
@@ -640,7 +640,7 @@ struct SemiNCAInfo {
Bucket;
SmallDenseSet<TreeNodePtr, 8> Visited;
SmallVector<TreeNodePtr, 8> Affected;
-#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
SmallVector<TreeNodePtr, 8> VisitedUnaffected;
#endif
};
@@ -915,7 +915,7 @@ struct SemiNCAInfo {
LLVM_DEBUG(dbgs() << "Deleting edge " << BlockNamePrinter(From) << " -> "
<< BlockNamePrinter(To) << "\n");
-#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
// Ensure that the edge was in fact deleted from the CFG before informing
// the DomTree about it.
// The check is O(N), so run it only in debug configuration.
diff --git a/llvm/include/llvm/Transforms/Scalar/JumpThreading.h b/llvm/include/llvm/Transforms/Scalar/JumpThreading.h
index 7d11fc0ad69384..a3f2ce23f7d9aa 100644
--- a/llvm/include/llvm/Transforms/Scalar/JumpThreading.h
+++ b/llvm/include/llvm/Transforms/Scalar/JumpThreading.h
@@ -89,9 +89,9 @@ class JumpThreadingPass : public PassInfoMixin<JumpThreadingPass> {
bool ChangedSinceLastAnalysisUpdate = false;
bool HasGuards = false;
#ifndef LLVM_ENABLE_ABI_BREAKING_CHECKS
- SmallSet<AssertingVH<const BasicBlock>, 16> LoopHeaders;
-#else
SmallPtrSet<const BasicBlock *, 16> LoopHeaders;
+#else
+ SmallSet<AssertingVH<const BasicBlock>, 16> LoopHeaders;
#endif
unsigned BBDupThreshold;
diff --git a/llvm/include/llvm/Transforms/Scalar/LoopPassManager.h b/llvm/include/llvm/Transforms/Scalar/LoopPassManager.h
index f55022fbff07c1..3858be05c61fa9 100644
--- a/llvm/include/llvm/Transforms/Scalar/LoopPassManager.h
+++ b/llvm/include/llvm/Transforms/Scalar/LoopPassManager.h
@@ -256,7 +256,7 @@ class LPMUpdater {
}
void setParentLoop(Loop *L) {
-#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
ParentL = L;
#endif
}
@@ -295,7 +295,7 @@ class LPMUpdater {
/// loops within them will be visited in postorder as usual for the loop pass
/// manager.
void addSiblingLoops(ArrayRef<Loop *> NewSibLoops) {
-#if LLVM_ENABLE_ABI_BREAKING_CHECKS && !defined(NDEBUG)
+#if defined(LLVM_ENABLE_ABI_BREAKING_CHECKS) && !defined(NDEBUG)
for (Loop *NewL : NewSibLoops)
assert(NewL->getParentLoop() == ParentL &&
"All of the new loops must be siblings of the current loop!");
@@ -347,7 +347,7 @@ class LPMUpdater {
const bool LoopNestMode;
bool LoopNestChanged;
-#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
// In debug builds we also track the parent loop to implement asserts even in
// the face of loop deletion.
Loop *ParentL;
diff --git a/llvm/include/llvm/Transforms/Utils/ScalarEvolutionExpander.h b/llvm/include/llvm/Transforms/Utils/ScalarEvolutionExpander.h
index 2806326593649e..faa1fa57faf6ce 100644
--- a/llvm/include/llvm/Transforms/Utils/ScalarEvolutionExpander.h
+++ b/llvm/include/llvm/Transforms/Utils/ScalarEvolutionExpander.h
@@ -168,7 +168,7 @@ class SCEVExpander : public SCEVVisitor<SCEVExpander, Value *> {
/// consistent when instructions are moved.
SmallVector<SCEVInsertPointGuard *, 8> InsertPointGuards;
-#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
const char *DebugType;
#endif
@@ -184,7 +184,7 @@ class SCEVExpander : public SCEVVisitor<SCEVExpander, Value *> {
Builder(se.getContext(), InstSimplifyFolder(DL),
IRBuilderCallbackInserter(
[this](Instruction *I) { rememberInstruction(I); })) {
-#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
DebugType = "";
#endif
}
@@ -194,7 +194,7 @@ class SCEVExpander : public SCEVVisitor<SCEVExpander, Value *> {
assert(InsertPointGuards.empty());
}
-#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
void setDebugType(const char *s) { DebugType = s; }
#endif
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
index 498debf0955980..64414e2d44e65a 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
@@ -11725,7 +11725,7 @@ void SelectionDAG::CreateTopologicalOrder(std::vector<SDNode *> &Order) {
}
}
-#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+#ifndef NDEBUG
void SelectionDAG::VerifyDAGDivergence() {
std::vector<SDNode *> TopoOrder;
CreateTopologicalOrder(TopoOrder);
diff --git a/llvm/lib/Passes/StandardInstrumentations.cpp b/llvm/lib/Passes/StandardInstrumentations.cpp
index 0ae3b2c84a5a1c..c37af27615dd91 100644
--- a/llvm/lib/Passes/StandardInstrumentations.cpp
+++ b/llvm/lib/Passes/StandardInstrumentations.cpp
@@ -1358,7 +1358,7 @@ void PreservedCFGCheckerInstrumentation::registerCallbacks(
bool Registered = false;
PIC.registerBeforeNonSkippedPassCallback([this, &MAM, Registered](
StringRef P, Any IR) mutable {
-#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
assert(&PassStack.emplace_back(P));
#endif
(void)this;
@@ -1387,7 +1387,7 @@ void PreservedCFGCheckerInstrumentation::registerCallbacks(
PIC.registerAfterPassInvalidatedCallback(
[this](StringRef P, const PreservedAnalyses &PassPA) {
-#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
assert(PassStack.pop_back_val() == P &&
"Before and After callbacks must correspond");
#endif
@@ -1396,7 +1396,7 @@ void PreservedCFGCheckerInstrumentation::registerCallbacks(
PIC.registerAfterPassCallback([this, &MAM](StringRef P, Any IR,
const PreservedAnalyses &PassPA) {
-#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
assert(PassStack.pop_back_val() == P &&
"Before and After callbacks must correspond");
#endif
diff --git a/llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp b/llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp
index 1ff3cd78aa9877..87f523ccdcccde 100644
--- a/llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp
+++ b/llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp
@@ -28,7 +28,7 @@
#include "llvm/Support/raw_ostream.h"
#include "llvm/Transforms/Utils/LoopUtils.h"
-#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
#define SCEV_DEBUG_WITH_TYPE(TYPE, X) DEBUG_WITH_TYPE(TYPE, X)
#else
#define SCEV_DEBUG_WITH_TYPE(TYPE, X)
diff --git a/mlir/include/mlir/Analysis/Presburger/PresburgerSpace.h b/mlir/include/mlir/Analysis/Presburger/PresburgerSpace.h
index 97573b6e453013..cff79579898710 100644
--- a/mlir/include/mlir/Analysis/Presburger/PresburgerSpace.h
+++ b/mlir/include/mlir/Analysis/Presburger/PresburgerSpace.h
@@ -75,7 +75,7 @@ class Identifier {
template <typename T>
explicit Identifier(T value)
: value(llvm::PointerLikeTypeTraits<T>::getAsVoidPointer(value)) {
-#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
idType = llvm::getTypeName<T>();
#endif
}
@@ -84,7 +84,7 @@ class Identifier {
/// the type of the identifier used to create it.
template <typename T>
T getValue() const {
-#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
assert(llvm::getTypeName<T>() == idType &&
"Identifier was initialized with a different type than the one used "
"to retrieve it.");
@@ -108,7 +108,7 @@ class Identifier {
/// The value of the identifier.
void *value = nullptr;
-#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
/// TypeID of the identifiers in space. This should be used in asserts only.
llvm::StringRef idType;
#endif
diff --git a/mlir/include/mlir/Dialect/Transform/Interfaces/TransformInterfaces.h b/mlir/include/mlir/Dialect/Transform/Interfaces/TransformInterfaces.h
index e51aac02936b57..43193e4cd4cf63 100644
--- a/mlir/include/mlir/Dialect/Transform/Interfaces/TransformInterfaces.h
+++ b/mlir/include/mlir/Dialect/Transform/Interfaces/TransformInterfaces.h
@@ -196,7 +196,7 @@ class TransformState {
/// should be emitted when the value is used.
using InvalidatedHandleMap = DenseMap<Value, std::function<void(Location)>>;
-#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
/// Debug only: A timestamp is associated with each transform IR value, so
/// that invalid iterator usage can be detected more reliably.
using TransformIRTimestampMapping = DenseMap<Value, int64_t>;
@@ -211,7 +211,7 @@ class TransformState {
ValueMapping values;
ValueMapping reverseValues;
-#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
TransformIRTimestampMapping timestamps;
void incrementTimestamp(Value value) { ++timestamps[value]; }
#endif // LLVM_ENABLE_ABI_BREAKING_CHECKS
@@ -248,7 +248,7 @@ class TransformState {
auto getPayloadOps(Value value) const {
ArrayRef<Operation *> view = getPayloadOpsView(value);
-#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
// Memorize the current timestamp and make sure that it has not changed
// when incrementing or dereferencing the iterator returned by this
// function. The timestamp is incremented when the "direct" mapping is
@@ -259,7 +259,7 @@ class TransformState {
// When ops are replaced/erased, they are replaced with nullptr (until
// the data structure is compacted). Do not enumerate these ops.
return llvm::make_filter_range(view, [=](Operation *op) {
-#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
[[maybe_unused]] bool sameTimestamp =
currentTimestamp == this->getMapping(value).timestamps.lookup(value);
assert(sameTimestamp && "iterator was invalidated during iteration");
@@ -277,7 +277,7 @@ class TransformState {
auto getPayloadValues(Value handleValue) const {
ArrayRef<Value> view = getPayloadValuesView(handleValue);
-#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
// Memorize the current timestamp and make sure that it has not changed
// when incrementing or dereferencing the iterator returned by this
// function. The timestamp is incremented when the "values" mapping is
diff --git a/mlir/include/mlir/IR/PDLPatternMatch.h.inc b/mlir/include/mlir/IR/PDLPatternMatch.h.inc
index 96ba98a850de00..66286ed7a4c898 100644
--- a/mlir/include/mlir/IR/PDLPatternMatch.h.inc
+++ b/mlir/include/mlir/IR/PDLPatternMatch.h.inc
@@ -625,7 +625,7 @@ template <typename PDLFnT, std::size_t... I>
void assertArgs(PatternRewriter &rewriter, ArrayRef<PDLValue> values,
std::index_sequence<I...>) {
// We only want to do verification in debug builds, same as with `assert`.
-#ifndef NDEBUG
+#if LLVM_ENABLE_ABI_BREAKING_CHECKS
using FnTraitsT = llvm::function_traits<PDLFnT>;
auto errorFn = [&](const Twine &msg) -> LogicalResult {
llvm::report_fatal_error(msg);
diff --git a/mlir/lib/Dialect/Transform/Interfaces/TransformInterfaces.cpp b/mlir/lib/Dialect/Transform/Interfaces/TransformInterfaces.cpp
index fdd968238667e2..91702ce7cc42b9 100644
--- a/mlir/lib/Dialect/Transform/Interfaces/TransformInterfaces.cpp
+++ b/mlir/lib/Dialect/Transform/Interfaces/TransformInterfaces.cpp
@@ -330,7 +330,7 @@ void transform::TransformState::forgetMapping(Value opHandle,
for (Operation *op : mappings.direct[opHandle])
dropMappingEntry(mappings.reverse, op, opHandle);
mappings.direct.erase(opHandle);
-#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
// Payload IR is removed from the mapping. This invalidates the respective
// iterators.
mappings.incrementTimestamp(opHandle);
@@ -342,7 +342,7 @@ void transform::TransformState::forgetMapping(Value opHandle,
for (Value resultHandle : resultHandles) {
Mappings &localMappings = getMapping(resultHandle);
dropMappingEntry(localMappings.values, resultHandle, opResult);
-#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
// Payload IR is removed from the mapping. This invalidates the respective
// iterators.
mappings.incrementTimestamp(resultHandle);
@@ -358,7 +358,7 @@ void transform::TransformState::forgetValueMapping(
for (Value payloadValue : mappings.reverseValues[valueHandle])
dropMappingEntry(mappings.reverseValues, payloadValue, valueHandle);
mappings.values.erase(valueHandle);
-#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
// Payload IR is removed from the mapping. This invalidates the respective
// iterators.
mappings.incrementTimestamp(valueHandle);
@@ -372,7 +372,7 @@ void transform::TransformState::forgetValueMapping(
dropMappingEntry(localMappings.direct, opHandle, payloadOp);
dropMappingEntry(localMappings.reverse, payloadOp, opHandle);
-#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
// Payload IR is removed from the mapping. This invalidates the respective
// iterators.
localMappings.incrementTimestamp(opHandle);
@@ -452,7 +452,7 @@ transform::TransformState::replacePayloadValue(Value value, Value replacement) {
// between the handles and the IR objects
if (!replacement) {
dropMappingEntry(mappings.values, handle, value);
-#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
// Payload IR is removed from the mapping. This invalidates the respective
// iterators.
mappings.incrementTimestamp(handle);
@@ -804,7 +804,7 @@ checkRepeatedConsumptionInOperand(ArrayRef<T> payload,
void transform::TransformState::compactOpHandles() {
for (Value handle : opHandlesToCompact) {
Mappings &mappings = getMapping(handle, /*allowOutOfScope=*/true);
-#if LLVM_ENABLE_ABI_BREAKING_CHECKS
+#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
if (llvm::find(mappings.direct[handle], nullptr) !=
mappings.direct[handle].end())
// Payload IR is removed from the mapping. This invalidates the respective
|
There hasn’t been a failing bot on the original PR, do you have some repro instructions about the issue your seeing? |
I've just updated my commit message to contain that info. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see #110918 supposedly fixes these issues, but we can reland the original PR along with those fixes
Seems like the Bazel config is a bit busted, we should update it: building a release build with these checks isn't really something that makes sense to me. (In the meantime: thanks for landing the revert though!) |
Actually it may be the opposite config (which is why I didn't have the issue locally): it may be a debug build with the LLVM_ENABLE_ABI_BREAKING_CHECKS disabled, I'll test this. |
I was indeed building with |
llvm#110923) …ead of #ifdef (llvm#110883)" This reverts commit 1905cdb, which causes lots of failures where LLVM doesn't have the right header guards. The errors can be seen on [BuildKite](https://buildkite.com/llvm-project/upstream-bazel/builds/112362#01924eae-231c-4d06-ba87-2c538cf40e04), where the source uses `#ifndef NDEBUG`, but the content in question is defined when `LLVM_ENABLE_ABI_BREAKING_CHECKS == 1`. For example, `llvm/include/llvm/Support/GenericDomTreeConstruction.h` has the following: ```cpp // Helper struct used during edge insertions. struct InsertionInfo { // ... #ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS SmallVector<TreeNodePtr, 8> VisitedUnaffected; #endif }; // ... InsertionInfo II; // ... #ifndef NDEBUG II.VisitedUnaffected.push_back(SuccTN); #endif ```
llvm#110923) …ead of #ifdef (llvm#110883)" This reverts commit 1905cdb, which causes lots of failures where LLVM doesn't have the right header guards. The errors can be seen on [BuildKite](https://buildkite.com/llvm-project/upstream-bazel/builds/112362#01924eae-231c-4d06-ba87-2c538cf40e04), where the source uses `#ifndef NDEBUG`, but the content in question is defined when `LLVM_ENABLE_ABI_BREAKING_CHECKS == 1`. For example, `llvm/include/llvm/Support/GenericDomTreeConstruction.h` has the following: ```cpp // Helper struct used during edge insertions. struct InsertionInfo { // ... #ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS SmallVector<TreeNodePtr, 8> VisitedUnaffected; #endif }; // ... InsertionInfo II; // ... #ifndef NDEBUG II.VisitedUnaffected.push_back(SuccTN); #endif ```
…ead of #ifdef (#110883)"
This reverts commit 1905cdb, which causes lots of failures where LLVM doesn't have the right header guards. The errors can be seen on BuildKite, where the source uses
#ifndef NDEBUG
, but the content in question is defined whenLLVM_ENABLE_ABI_BREAKING_CHECKS == 1
.For example,
llvm/include/llvm/Support/GenericDomTreeConstruction.h
has the following: