Skip to content

[Misc] Use LLVM_ENABLE_ABI_BREAKING_CHECKS correctly #94212

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

Merged
merged 1 commit into from
Jun 10, 2024

Conversation

paperchalice
Copy link
Contributor

LLVM_ENABLE_ABI_BREAKING_CHECKS is always defined:
https://github.com/llvm/llvm-project/blob/72c901f5e59477e568b1b04dae9de753b9d1f6f3/llvm/include/llvm/Config/abi-breaking.h.cmake#L16C2-L16C15
It uses cmakedefine01 rather than cmakedefine, so LLVM_ENABLE_ABI_BREAKING_CHECKS is always defined,
so the preprocessed code is probably not what the author wanted.

@llvmbot
Copy link
Member

llvmbot commented Jun 3, 2024

@llvm/pr-subscribers-llvm-support

@llvm/pr-subscribers-llvm-transforms

Author: None (paperchalice)

Changes

LLVM_ENABLE_ABI_BREAKING_CHECKS is always defined:
https://github.com/llvm/llvm-project/blob/72c901f5e59477e568b1b04dae9de753b9d1f6f3/llvm/include/llvm/Config/abi-breaking.h.cmake#L16C2-L16C15
It uses cmakedefine01 rather than cmakedefine, so LLVM_ENABLE_ABI_BREAKING_CHECKS is always defined,
so the preprocessed code is probably not what the author wanted.


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

7 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/JumpThreading.h (+1-1)
  • (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)
diff --git a/llvm/include/llvm/Passes/StandardInstrumentations.h b/llvm/include/llvm/Passes/StandardInstrumentations.h
index 84d1b541171bf..9de8ba5b8bf51 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 29b5f9159c68e..b9d398fee2b74 100644
--- a/llvm/include/llvm/Support/GenericDomTreeConstruction.h
+++ b/llvm/include/llvm/Support/GenericDomTreeConstruction.h
@@ -638,7 +638,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
   };
@@ -913,7 +913,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/JumpThreading.h b/llvm/include/llvm/Transforms/Scalar/JumpThreading.h
index 65d43775bdc1d..290e5a1cc337f 100644
--- a/llvm/include/llvm/Transforms/Scalar/JumpThreading.h
+++ b/llvm/include/llvm/Transforms/Scalar/JumpThreading.h
@@ -88,7 +88,7 @@ class JumpThreadingPass : public PassInfoMixin<JumpThreadingPass> {
   std::optional<BranchProbabilityInfo *> BPI;
   bool ChangedSinceLastAnalysisUpdate = false;
   bool HasGuards = false;
-#ifndef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if !LLVM_ENABLE_ABI_BREAKING_CHECKS
   SmallPtrSet<const BasicBlock *, 16> LoopHeaders;
 #else
   SmallSet<AssertingVH<const BasicBlock>, 16> LoopHeaders;
diff --git a/llvm/include/llvm/Transforms/Scalar/LoopPassManager.h b/llvm/include/llvm/Transforms/Scalar/LoopPassManager.h
index 6aab1f98e6781..227f6dfb8362c 100644
--- a/llvm/include/llvm/Transforms/Scalar/LoopPassManager.h
+++ b/llvm/include/llvm/Transforms/Scalar/LoopPassManager.h
@@ -286,7 +286,7 @@ class LPMUpdater {
   }
 
   void setParentLoop(Loop *L) {
-#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS
+#if LLVM_ENABLE_ABI_BREAKING_CHECKS
     ParentL = L;
 #endif
   }
@@ -377,7 +377,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 62c1e15a9a60e..468b50092efcf 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 c7adc7668b9a1..b23d5fe245481 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 0feea0a4233cd..b79594fae00e6 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)

Copy link
Contributor

@sheredom sheredom left a comment

Choose a reason for hiding this comment

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

The change seems legit to me. I dunno exactly why I'm the reviewer, but from the CMake that I am (un)fortunate to know, this does seem like an oversight in what was intended.

@paperchalice paperchalice merged commit 291b415 into llvm:main Jun 10, 2024
10 checks passed
@steelannelida
Copy link

Some builds are broken by this https://buildkite.com/llvm-project/upstream-bazel/builds/100119#0190021b-2e6a-4fe9-bcfa-ce1bb70ec671

I'm working on a fix.

@RKSimon
Copy link
Collaborator

RKSimon commented Jun 10, 2024

You've missed a log of defined(LLVM_ENABLE_ABI_BREAKING_CHECKS) patterns - please revert and redo this patch cleanly

paperchalice added a commit that referenced this pull request Jun 10, 2024
RKSimon added a commit that referenced this pull request Jun 10, 2024
…ly (#94212)

The patch didn't consistently clean up `#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS` and '#if defined(LLVM_ENABLE_ABI_BREAKING_CHECKS)' paths, causing a lot of build failures
paperchalice added a commit that referenced this pull request Jun 10, 2024
Reverts #94212
Some codes assume that `NDEBUG` implies
`LLVM_ENABLE_ABI_BREAKING_CHECKS`, thus #94212 breaks some build bots.
@steelannelida
Copy link

#94985

Lukacma pushed a commit to Lukacma/llvm-project that referenced this pull request Jun 12, 2024
…ly (llvm#94212)

The patch didn't consistently clean up `#ifdef LLVM_ENABLE_ABI_BREAKING_CHECKS` and '#if defined(LLVM_ENABLE_ABI_BREAKING_CHECKS)' paths, causing a lot of build failures
Lukacma pushed a commit to Lukacma/llvm-project that referenced this pull request Jun 12, 2024
…#94982)

Reverts llvm#94212
Some codes assume that `NDEBUG` implies
`LLVM_ENABLE_ABI_BREAKING_CHECKS`, thus llvm#94212 breaks some build bots.
@HerrCai0907 HerrCai0907 mentioned this pull request Jun 13, 2024
clayborg pushed a commit to clayborg/llvm-project that referenced this pull request Jun 20, 2024
…#94982)

Reverts llvm#94212
Some codes assume that `NDEBUG` implies
`LLVM_ENABLE_ABI_BREAKING_CHECKS`, thus llvm#94212 breaks some build bots.
clayborg pushed a commit to clayborg/llvm-project that referenced this pull request Jun 24, 2024
…#94982)

Reverts llvm#94212
Some codes assume that `NDEBUG` implies
`LLVM_ENABLE_ABI_BREAKING_CHECKS`, thus llvm#94212 breaks some build bots.
@paperchalice paperchalice deleted the ifdef-if branch September 22, 2024 13:00
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.

5 participants