Skip to content
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

Fix LLVM_ENABLE_ABI_BREAKING_CHECKS macro check: use #if instead of #ifdef #110185

Merged
merged 1 commit into from
Sep 27, 2024

Conversation

joker-eph
Copy link
Collaborator

This macros is always defined: either 0 or 1. The correct pattern is to use #if.

…ifdef

This macros is always defined: either 0 or 1. The correct pattern is to use #if.
@llvmbot
Copy link
Member

llvmbot commented Sep 26, 2024

@llvm/pr-subscribers-llvm-support
@llvm/pr-subscribers-mlir-presburger

@llvm/pr-subscribers-mlir

Author: Mehdi Amini (joker-eph)

Changes

This macros is always defined: either 0 or 1. The correct pattern is to use #if.


Full diff: https://github.com/llvm/llvm-project/pull/110185.diff

9 Files Affected:

  • (modified) llvm/include/llvm/Passes/StandardInstrumentations.h (+1-1)
  • (modified) llvm/include/llvm/Support/GenericDomTreeConstruction.h (+2-2)
  • (modified) llvm/include/llvm/Transforms/Scalar/LoopPassManager.h (+2-2)
  • (modified) llvm/include/llvm/Transforms/Utils/ScalarEvolutionExpander.h (+3-3)
  • (modified) llvm/lib/Passes/StandardInstrumentations.cpp (+3-3)
  • (modified) llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp (+1-1)
  • (modified) mlir/include/mlir/Analysis/Presburger/PresburgerSpace.h (+3-3)
  • (modified) mlir/include/mlir/Dialect/Transform/Interfaces/TransformInterfaces.h (+5-5)
  • (modified) mlir/lib/Dialect/Transform/Interfaces/TransformInterfaces.cpp (+6-6)
diff --git a/llvm/include/llvm/Passes/StandardInstrumentations.h b/llvm/include/llvm/Passes/StandardInstrumentations.h
index fa9c744294a666..80eedc52bc324e 100644
--- a/llvm/include/llvm/Passes/StandardInstrumentations.h
+++ b/llvm/include/llvm/Passes/StandardInstrumentations.h
@@ -171,7 +171,7 @@ class PreservedCFGCheckerInstrumentation {
                     FunctionAnalysisManager::Invalidator &);
   };
 
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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 9aab5ec60f4a25..2e21bdc9fce2d5 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;
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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");
 
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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/LoopPassManager.h b/llvm/include/llvm/Transforms/Scalar/LoopPassManager.h
index 3858be05c61fa9..db479f55d9b034 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) {
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if LLVM_ENABLE_ABI_BREAKING_CHECKS
     ParentL = L;
 #endif
   }
@@ -347,7 +347,7 @@ class LPMUpdater {
   const bool LoopNestMode;
   bool LoopNestChanged;
 
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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 62c1e15a9a60e1..468b50092efcf7 100644
--- a/llvm/include/llvm/Transforms/Utils/ScalarEvolutionExpander.h
+++ b/llvm/include/llvm/Transforms/Utils/ScalarEvolutionExpander.h
@@ -167,7 +167,7 @@ class SCEVExpander : public SCEVVisitor<SCEVExpander, Value *> {
   /// consistent when instructions are moved.
   SmallVector<SCEVInsertPointGuard *, 8> InsertPointGuards;
 
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if LLVM_ENABLE_ABI_BREAKING_CHECKS
   const char *DebugType;
 #endif
 
@@ -183,7 +183,7 @@ class SCEVExpander : public SCEVVisitor<SCEVExpander, Value *> {
         Builder(se.getContext(), InstSimplifyFolder(DL),
                 IRBuilderCallbackInserter(
                     [this](Instruction *I) { rememberInstruction(I); })) {
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if LLVM_ENABLE_ABI_BREAKING_CHECKS
     DebugType = "";
 #endif
   }
@@ -193,7 +193,7 @@ class SCEVExpander : public SCEVVisitor<SCEVExpander, Value *> {
     assert(InsertPointGuards.empty());
   }
 
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if LLVM_ENABLE_ABI_BREAKING_CHECKS
   void setDebugType(const char *s) { DebugType = s; }
 #endif
 
diff --git a/llvm/lib/Passes/StandardInstrumentations.cpp b/llvm/lib/Passes/StandardInstrumentations.cpp
index 036484c9c1c0c4..a545ae5862397f 100644
--- a/llvm/lib/Passes/StandardInstrumentations.cpp
+++ b/llvm/lib/Passes/StandardInstrumentations.cpp
@@ -1357,7 +1357,7 @@ void PreservedCFGCheckerInstrumentation::registerCallbacks(
   bool Registered = false;
   PIC.registerBeforeNonSkippedPassCallback([this, &MAM, Registered](
                                                StringRef P, Any IR) mutable {
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if LLVM_ENABLE_ABI_BREAKING_CHECKS
     assert(&PassStack.emplace_back(P));
 #endif
     (void)this;
@@ -1386,7 +1386,7 @@ void PreservedCFGCheckerInstrumentation::registerCallbacks(
 
   PIC.registerAfterPassInvalidatedCallback(
       [this](StringRef P, const PreservedAnalyses &PassPA) {
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if LLVM_ENABLE_ABI_BREAKING_CHECKS
         assert(PassStack.pop_back_val() == P &&
                "Before and After callbacks must correspond");
 #endif
@@ -1395,7 +1395,7 @@ void PreservedCFGCheckerInstrumentation::registerCallbacks(
 
   PIC.registerAfterPassCallback([this, &MAM](StringRef P, Any IR,
                                              const PreservedAnalyses &PassPA) {
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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 0927a3015818fd..2119320566902d 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"
 
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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 cff79579898710..97573b6e453013 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)) {
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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 {
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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;
 
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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 43193e4cd4cf63..e51aac02936b57 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)>>;
 
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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;
 
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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);
 
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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) {
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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);
 
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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/lib/Dialect/Transform/Interfaces/TransformInterfaces.cpp b/mlir/lib/Dialect/Transform/Interfaces/TransformInterfaces.cpp
index 91702ce7cc42b9..fdd968238667e2 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);
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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);
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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);
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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);
 
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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);
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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);
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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

@llvmbot
Copy link
Member

llvmbot commented Sep 26, 2024

@llvm/pr-subscribers-llvm-transforms

Author: Mehdi Amini (joker-eph)

Changes

This macros is always defined: either 0 or 1. The correct pattern is to use #if.


Full diff: https://github.com/llvm/llvm-project/pull/110185.diff

9 Files Affected:

  • (modified) llvm/include/llvm/Passes/StandardInstrumentations.h (+1-1)
  • (modified) llvm/include/llvm/Support/GenericDomTreeConstruction.h (+2-2)
  • (modified) llvm/include/llvm/Transforms/Scalar/LoopPassManager.h (+2-2)
  • (modified) llvm/include/llvm/Transforms/Utils/ScalarEvolutionExpander.h (+3-3)
  • (modified) llvm/lib/Passes/StandardInstrumentations.cpp (+3-3)
  • (modified) llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp (+1-1)
  • (modified) mlir/include/mlir/Analysis/Presburger/PresburgerSpace.h (+3-3)
  • (modified) mlir/include/mlir/Dialect/Transform/Interfaces/TransformInterfaces.h (+5-5)
  • (modified) mlir/lib/Dialect/Transform/Interfaces/TransformInterfaces.cpp (+6-6)
diff --git a/llvm/include/llvm/Passes/StandardInstrumentations.h b/llvm/include/llvm/Passes/StandardInstrumentations.h
index fa9c744294a666..80eedc52bc324e 100644
--- a/llvm/include/llvm/Passes/StandardInstrumentations.h
+++ b/llvm/include/llvm/Passes/StandardInstrumentations.h
@@ -171,7 +171,7 @@ class PreservedCFGCheckerInstrumentation {
                     FunctionAnalysisManager::Invalidator &);
   };
 
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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 9aab5ec60f4a25..2e21bdc9fce2d5 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;
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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");
 
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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/LoopPassManager.h b/llvm/include/llvm/Transforms/Scalar/LoopPassManager.h
index 3858be05c61fa9..db479f55d9b034 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) {
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if LLVM_ENABLE_ABI_BREAKING_CHECKS
     ParentL = L;
 #endif
   }
@@ -347,7 +347,7 @@ class LPMUpdater {
   const bool LoopNestMode;
   bool LoopNestChanged;
 
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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 62c1e15a9a60e1..468b50092efcf7 100644
--- a/llvm/include/llvm/Transforms/Utils/ScalarEvolutionExpander.h
+++ b/llvm/include/llvm/Transforms/Utils/ScalarEvolutionExpander.h
@@ -167,7 +167,7 @@ class SCEVExpander : public SCEVVisitor<SCEVExpander, Value *> {
   /// consistent when instructions are moved.
   SmallVector<SCEVInsertPointGuard *, 8> InsertPointGuards;
 
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if LLVM_ENABLE_ABI_BREAKING_CHECKS
   const char *DebugType;
 #endif
 
@@ -183,7 +183,7 @@ class SCEVExpander : public SCEVVisitor<SCEVExpander, Value *> {
         Builder(se.getContext(), InstSimplifyFolder(DL),
                 IRBuilderCallbackInserter(
                     [this](Instruction *I) { rememberInstruction(I); })) {
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if LLVM_ENABLE_ABI_BREAKING_CHECKS
     DebugType = "";
 #endif
   }
@@ -193,7 +193,7 @@ class SCEVExpander : public SCEVVisitor<SCEVExpander, Value *> {
     assert(InsertPointGuards.empty());
   }
 
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if LLVM_ENABLE_ABI_BREAKING_CHECKS
   void setDebugType(const char *s) { DebugType = s; }
 #endif
 
diff --git a/llvm/lib/Passes/StandardInstrumentations.cpp b/llvm/lib/Passes/StandardInstrumentations.cpp
index 036484c9c1c0c4..a545ae5862397f 100644
--- a/llvm/lib/Passes/StandardInstrumentations.cpp
+++ b/llvm/lib/Passes/StandardInstrumentations.cpp
@@ -1357,7 +1357,7 @@ void PreservedCFGCheckerInstrumentation::registerCallbacks(
   bool Registered = false;
   PIC.registerBeforeNonSkippedPassCallback([this, &MAM, Registered](
                                                StringRef P, Any IR) mutable {
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if LLVM_ENABLE_ABI_BREAKING_CHECKS
     assert(&PassStack.emplace_back(P));
 #endif
     (void)this;
@@ -1386,7 +1386,7 @@ void PreservedCFGCheckerInstrumentation::registerCallbacks(
 
   PIC.registerAfterPassInvalidatedCallback(
       [this](StringRef P, const PreservedAnalyses &PassPA) {
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if LLVM_ENABLE_ABI_BREAKING_CHECKS
         assert(PassStack.pop_back_val() == P &&
                "Before and After callbacks must correspond");
 #endif
@@ -1395,7 +1395,7 @@ void PreservedCFGCheckerInstrumentation::registerCallbacks(
 
   PIC.registerAfterPassCallback([this, &MAM](StringRef P, Any IR,
                                              const PreservedAnalyses &PassPA) {
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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 0927a3015818fd..2119320566902d 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"
 
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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 cff79579898710..97573b6e453013 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)) {
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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 {
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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;
 
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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 43193e4cd4cf63..e51aac02936b57 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)>>;
 
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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;
 
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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);
 
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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) {
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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);
 
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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/lib/Dialect/Transform/Interfaces/TransformInterfaces.cpp b/mlir/lib/Dialect/Transform/Interfaces/TransformInterfaces.cpp
index 91702ce7cc42b9..fdd968238667e2 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);
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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);
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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);
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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);
 
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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);
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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);
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if 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

Copy link
Collaborator

@dwblaikie dwblaikie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM - this'll be an ABI break, of course - but we don't have an ABI stability between releases, so I don't think this would adversely affect anyone. We just have to be consistent within a release or version, and we were consistent (though inefficient) before this patch and we'll be consistent (though more efficient/lower memory usage, etc) afterwards.

@joker-eph joker-eph merged commit 5e98136 into llvm:main Sep 27, 2024
11 of 13 checks passed
@joker-eph joker-eph deleted the fix_macro branch September 27, 2024 10:07
@llvm-ci
Copy link
Collaborator

llvm-ci commented Sep 27, 2024

LLVM Buildbot has detected a new failure on builder openmp-offload-libc-amdgpu-runtime running on omp-vega20-1 while building llvm,mlir at step 10 "Add check check-offload".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/73/builds/6247

Here is the relevant piece of the build log for the reference
Step 10 (Add check check-offload) failure: 1200 seconds without output running [b'ninja', b'-j 32', b'check-offload'], attempting to kill
...
PASS: libomptarget :: x86_64-unknown-linux-gnu-LTO :: offloading/bug49779.cpp (866 of 879)
PASS: libomptarget :: x86_64-unknown-linux-gnu-LTO :: offloading/bug53727.cpp (867 of 879)
PASS: libomptarget :: x86_64-unknown-linux-gnu-LTO :: offloading/bug50022.cpp (868 of 879)
PASS: libomptarget :: x86_64-unknown-linux-gnu-LTO :: offloading/test_libc.cpp (869 of 879)
PASS: libomptarget :: x86_64-unknown-linux-gnu-LTO :: offloading/wtime.c (870 of 879)
PASS: libomptarget :: x86_64-unknown-linux-gnu :: offloading/bug49021.cpp (871 of 879)
PASS: libomptarget :: x86_64-unknown-linux-gnu :: offloading/std_complex_arithmetic.cpp (872 of 879)
PASS: libomptarget :: x86_64-unknown-linux-gnu-LTO :: offloading/complex_reduction.cpp (873 of 879)
PASS: libomptarget :: x86_64-unknown-linux-gnu-LTO :: offloading/bug49021.cpp (874 of 879)
PASS: libomptarget :: x86_64-unknown-linux-gnu-LTO :: offloading/std_complex_arithmetic.cpp (875 of 879)
command timed out: 1200 seconds without output running [b'ninja', b'-j 32', b'check-offload'], attempting to kill
process killed by signal 9
program finished with exit code -1
elapsedTime=1237.003004

hanhanW added a commit to iree-org/llvm-project that referenced this pull request Sep 27, 2024
@hanhanW
Copy link
Contributor

hanhanW commented Sep 27, 2024

The change breaks downstream IREE project: https://github.com/iree-org/iree/actions/runs/11074921339/job/30774781373

It is not captured in CI because LLVM isn't building with whatever combination of asserts enabled and NDEBUG. I'd like ask for a revert because other places are not updated and it does not look like a trivial fix to me. E.g.,

#if defined(LLVM_ENABLE_ABI_BREAKING_CHECKS) && !defined(NDEBUG)
is not updated.

There are other issues too. E.g.,

#if LLVM_ENABLE_ABI_BREAKING_CHECKS
SmallVector<TreeNodePtr, 8> VisitedUnaffected;
#endif

#ifndef NDEBUG
II.VisitedUnaffected.push_back(SuccTN);
#endif

The variable is declared if one define is set, and it uses the variable if a different define is set.

Can we revert the change?

@joker-eph
Copy link
Collaborator Author

Sure, happy to revert of course!
But we need a repro because I want to re-land, can you provide a config I can build upstream that will reproduce the issues? Thanks!

joker-eph added a commit that referenced this pull request Sep 27, 2024
…ead of #ifdef" (#110310)

Reverts #110185

There are inconsistencies in some of these macros, which unfortunately
isn't caught by a single upstream bot.
@joker-eph
Copy link
Collaborator Author

I think I can reproduce with a release build and -DLLVM_ABI_BREAKING_CHECKS=FORCE_ON.

Note that this is a bit of a weird config, you may want to check your downstream settings.

@ScottTodd
Copy link
Member

We do jump through some hoops with asserts and NDEBUG downstream, yeah: https://github.com/iree-org/iree/blob/ff1b8b0e433372b73ad6abcaa101ae0dc2fd639d/CMakeLists.txt#L477-L522. I'm seeing if I can find better upstream repro steps...

@ScottTodd
Copy link
Member

For reference: https://llvm.org/docs/CMake.html#llvm-related-variables

LLVM_ABI_BREAKING_CHECKS:STRING
Used to decide if LLVM should be built with ABI breaking checks or
not. Allowed values are WITH_ASSERTS (default), FORCE_ON and
FORCE_OFF. WITH_ASSERTS turns on ABI breaking checks in an
assertion enabled build. FORCE_ON (FORCE_OFF) turns them on
(off) irrespective of whether normal (NDEBUG-based) assertions are
enabled or not. A version of LLVM built with ABI breaking checks
is not ABI compatible with a version built without it.

@joker-eph
Copy link
Collaborator Author

I don't think I need more repro steps, but you likely wanna check what your custom cmake ends up with LLVM inferring LLVM_ABI_BREAKING_CHECKS to be on?

Sterling-Augustine pushed a commit to Sterling-Augustine/llvm-project that referenced this pull request Sep 27, 2024
…ifdef (llvm#110185)

This macros is always defined: either 0 or 1. The correct pattern is to
use #if.
Sterling-Augustine pushed a commit to Sterling-Augustine/llvm-project that referenced this pull request Sep 27, 2024
…ead of #ifdef" (llvm#110310)

Reverts llvm#110185

There are inconsistencies in some of these macros, which unfortunately
isn't caught by a single upstream bot.
puja2196 pushed a commit to puja2196/LLVM-tutorial that referenced this pull request Sep 30, 2024
…ead of #ifdef" (#110310)

Reverts llvm/llvm-project#110185

There are inconsistencies in some of these macros, which unfortunately
isn't caught by a single upstream bot.
puja2196 pushed a commit to puja2196/LLVM-tutorial that referenced this pull request Oct 2, 2024
…ead of #ifdef" (#110310)

Reverts llvm/llvm-project#110185

There are inconsistencies in some of these macros, which unfortunately
isn't caught by a single upstream bot.
joker-eph added a commit that referenced this pull request Oct 2, 2024
…ifdef (#110883)

This macros is always defined: either 0 or 1. The correct pattern is to
use #if.

Reapply #110185 with fixes.
@joker-eph
Copy link
Collaborator Author

Relanded in 1905cdb

joker-eph added a commit that referenced this pull request Oct 2, 2024
…ifdef (#110938)

This macros is always defined: either 0 or 1. The correct pattern is to
use #if.

Re-apply #110185 with more fixes for debug build with the ABI breaking
checks disabled.
Sterling-Augustine pushed a commit to Sterling-Augustine/llvm-project that referenced this pull request Oct 3, 2024
…ifdef (llvm#110883)

This macros is always defined: either 0 or 1. The correct pattern is to
use #if.

Reapply llvm#110185 with fixes.
xgupta pushed a commit to xgupta/llvm-project that referenced this pull request Oct 4, 2024
…ifdef (llvm#110185)

This macros is always defined: either 0 or 1. The correct pattern is to
use #if.
xgupta pushed a commit to xgupta/llvm-project that referenced this pull request Oct 4, 2024
…ead of #ifdef" (llvm#110310)

Reverts llvm#110185

There are inconsistencies in some of these macros, which unfortunately
isn't caught by a single upstream bot.
xgupta pushed a commit to xgupta/llvm-project that referenced this pull request Oct 4, 2024
…ifdef (llvm#110883)

This macros is always defined: either 0 or 1. The correct pattern is to
use #if.

Reapply llvm#110185 with fixes.
xgupta pushed a commit to xgupta/llvm-project that referenced this pull request Oct 4, 2024
…ifdef (llvm#110938)

This macros is always defined: either 0 or 1. The correct pattern is to
use #if.

Re-apply llvm#110185 with more fixes for debug build with the ABI breaking
checks disabled.
@DimitryAndric
Copy link
Collaborator

After 1905cdb, I'm running into segfaults when assertions are disabled (NDEBUG is defined) and LLVM_ENABLE_ABI_BREAKING_CHECKS=1. The problem is that llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp has in SelectionDAGISel::initializeAnalysisResults:

#if !defined(NDEBUG) && LLVM_ENABLE_ABI_BREAKING_CHECKS
  TTI = &FAM.getResult<TargetIRAnalysis>(Fn);
#endif

but all the calls to VerifyDAGDivergence in SelectionDAGISel::CodeGenAndEmitDAG are first checked by accessing TTI:

#if LLVM_ENABLE_ABI_BREAKING_CHECKS
  if (TTI->hasBranchDivergence())
    CurDAG->VerifyDAGDivergence();
#endif

Since TTI is initialized with nullptr in the SelectionDAGISel class declaration, this access segfaults.

@joker-eph
Copy link
Collaborator Author

when assertions are disabled (NDEBUG is defined) and LLVM_ENABLE_ABI_BREAKING_CHECKS=1

This seems like a weird configuration to me (if I understand correctly): what is the motivation to build LLVM with this way?

Feel free to make the code consistent though, it won't hurt.

@DimitryAndric
Copy link
Collaborator

There is no particular reason, except that this is the configuration you get when building FreeBSD with WITHOUT_LLVM_ASSERTIONS enabled. We are using a few pre-generated header files, which set LLVM_ENABLE_ABI_BREAKING_CHECKS=1, but I was not aware that this define is coupled to NDEBUG. If that is the case, there should really only be checks for LLVM_ENABLE_ABI_BREAKING_CHECKS?

@DimitryAndric
Copy link
Collaborator

So in llvm/cmake/modules/HandleLLVMOptions.cmake there is this fragment:

if( uppercase_LLVM_ABI_BREAKING_CHECKS STREQUAL "WITH_ASSERTS" )
  if( LLVM_ENABLE_ASSERTIONS )
    set( LLVM_ENABLE_ABI_BREAKING_CHECKS 1 )
  endif()
elseif( uppercase_LLVM_ABI_BREAKING_CHECKS STREQUAL "FORCE_ON" )
  set( LLVM_ENABLE_ABI_BREAKING_CHECKS 1 )
elseif( uppercase_LLVM_ABI_BREAKING_CHECKS STREQUAL "FORCE_OFF" )
  # We don't need to do anything special to turn off ABI breaking checks.
elseif( NOT DEFINED LLVM_ABI_BREAKING_CHECKS )
  # Treat LLVM_ABI_BREAKING_CHECKS like "FORCE_OFF" when it has not been
  # defined.
else()
  message(FATAL_ERROR "Unknown value for LLVM_ABI_BREAKING_CHECKS: \"${LLVM_ABI_BREAKING_CHECKS}\"!")
endif()

I thought we could maybe remove all NDEBUG checks when used in combination with LLVM_ENABLE_ABI_BREAKING_CHECKS, but apparently you can "force on" LLVM_ENABLE_ABI_BREAKING_CHECKS even when LLVM_ENABLE_ASSERTIONS is off.

So for some reason, this is still a valid combination, although it does not really make sense to me. :)

@joker-eph
Copy link
Collaborator Author

Yes this is a valid combination, I just don't understand why anyone would want it in practice. It's been a while, but from memory I see the ABI_BREAKING_CHECKS as checks that are extras on top of the assertions.

@DimitryAndric
Copy link
Collaborator

Yeah, but that is a fix for another day. For now I have submitted #113588.

@joker-eph
Copy link
Collaborator Author

What « fix » do you refer to for another day?

@DimitryAndric
Copy link
Collaborator

What « fix » do you refer to for another day?

Sorry, I meant that if the combination of NDEBUG and LLVM_ENABLE_ABI_BREAKING_CHECKS does not make sense, we could adjust the CMake files for that, and get rid of the FORCE_ON and FORCE_OFF settings, only ever enabling LLVM_ENABLE_ABI_BREAKING_CHECKS if NDEBUG is not enabled. Then you could get rid of the NDEBUG checks here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants