Cleanup some problems with LLVM_ENABLE_DUMP in release builds.

[hintonda]

Summary

Cleanup some problems with LLVM_ENABLE_DUMP in release builds

Removed a few unneeded tests for NDEBUG and replace others with
#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP).

One issue involved ifdef'ing away a char* field from a struct, and changing
tablegen to handle it correctly, which might not be the best way to handle it.

[hintonda]

• Set LLVM_ENABLE_DUMP in config.h instead of passing it on the command line.

[MatzeB]

Thanks for working on it. Looks fine to me, but I think the cmake logic needs adjustment.

CMakeLists.txt

399–402

This is fragile, as for any subsequent cmake runs the value for LLVM_ENABLE_DUMP is already set in the cmake cache and won't change even if the user modifies LLVM_ENABLE_ASSERTIONS.

We rather want cmake code that sets the define if either this option or LLVM_ENABLE_ASSERTIONS is enabled.

lib/CodeGen/ScoreboardHazardRecognizer.cpp

35

Is this to silence a warning about that member being unused? Maybe we better put an #ifdef around the member then?

[hintonda]

CMakeLists.txt

399–402

So, are you saying it should always be set for +Asserts builds?

The email thread seemed to indicate they should vary independently.

lib/CodeGen/ScoreboardHazardRecognizer.cpp

35

Yes, and I considered that, but you'd also have to #ifdef away the ctor initializer which seems really ugly to me. Please let me know which you prefer.

[MatzeB]

CMakeLists.txt

399–402

Yes.
I cannot think of any reason why you would not want the dump functions in an asserts build. Also currently we use a !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP) in the cpp files so we have to enable the dumpers in assert builds of llvm. Admittedly after this whole thread I wonder if we shouldn't simplify this to just #ifdef LLVM_ENABLE_DUMP but we better do that another time in a separate patch.

lib/CodeGen/ScoreboardHazardRecognizer.cpp

35

It's fine like this.

[hintonda]

• Always set LLVM_ENABLE_DUMP=ON for +Asserts builds.

[MatzeB]

Thanks, LGTM

[mehdi_amini]

Thanks for fixing this :)

CMakeLists.txt

399–402

+1:

The code relate to dump should only be guarded by #ifdef LLVM_ENABLE_DUMP , and this should be on by default in Debug build, and then it is flexible enough and everyone is happy :)

[mehdi_amini]

Ideally, I think all the headers would hide the method behind #ifdef LLVM_ENABLE_DUMP , so that it wouldn't be a linker failure but a compile time failure!

[MatzeB]

In D38306#882704, @mehdi_amini wrote:

Ideally, I think all the headers would hide the method behind #ifdef LLVM_ENABLE_DUMP , so that it wouldn't be a linker failure but a compile time failure!

Yes. I think this wasn't done in the past because we did not want to have defined(NDEBUG) checks in the public headers because users could obviously set this flag different from what we had when LLVM was building. But as soon as we push that logic into a LLVM_ENABLE_DUMP flag and make it not depend on NDEBUG anymore we can go back to hiding the definitions in the headers as users should get the correct setting via LLVMs config.h.

[hintonda]

llvm/trunk/CMakeLists.txt

400 ↗

(On Diff #116884)

Just noticed that this doesn't actually update the cache. However, that doesn't matter in this case since the updated variable is what is actually used.

I'll fix this shortly so it won't confuse anyone looking at the cache.

[MatzeB]

llvm/trunk/CMakeLists.txt

400 ↗

(On Diff #116884)

Yep it does not update the cache and that is a good thing in this situation IMO... I'd rather not see cmake change setting B just because I switched setting A...

[MatzeB]

llvm/trunk/CMakeLists.txt

400 ↗

(On Diff #116884)

Or put another way: That's why I used the "Enable Dump function in release builds" description to indicate the flag has no effect in debug builds because the feature is enabled anyway.

[hintonda]

llvm/trunk/CMakeLists.txt

400 ↗

(On Diff #116884)

Yep it does not update the cache and that is a good thing in this situation IMO... I'd rather not see cmake change setting B just because I switched setting A...

We can do this by adding an additional variable, i.e., cache the passed variable, but determine LLVM_ENABLE_DUMP on the fly -- using the same name for both can be confusing.

400 ↗

(On Diff #116884)

Or put another way: That's why I used the "Enable Dump function in release builds" description to indicate the flag has no effect in debug builds because the feature is enabled anyway.

That's a good point. Once we remove the test for !defined(NDEBUG), LLVM_ENABLE_DUMP will be used for both, so we'll need to change that.

[MatzeB]

llvm/trunk/CMakeLists.txt

400 ↗

(On Diff #116884)

Sure, any good name suggestions? I'll throw LLVM_FORCE_ENABLE_DUMP into the bikeshed discussion.

[mehdi_amini]

llvm/trunk/CMakeLists.txt

400 ↗

(On Diff #116884)

Do we need this?
What about changing is in the config.h:

/* Define if LLVM_ENABLE_DUMP is enabled or not DNDEBUG */
#cmakedefine LLVM_ENABLE_DUMP
#ifndef NDEBUG
#ifndef LLVM_ENABLE_DUMP
#define LLVM_ENABLE_DUMP
#endif
#endif

Wouldn't this cover all the uses?

[MatzeB]

llvm/trunk/CMakeLists.txt

400 ↗

(On Diff #116884)

This solution gets us back into the trouble of picking up the users NDEBUG setting in the header and not necessarily the one used when compiling llvm.

[MatzeB]

llvm/trunk/CMakeLists.txt

400 ↗

(On Diff #116884)

Then again this is hardly the first instance of this problem in LLVM and we could start fixing it by adding something like LLVM_ENABLE_ASSERTION and use that instead of NDEBUG in our headers.

[hintonda]

llvm/trunk/CMakeLists.txt

400 ↗

(On Diff #116884)

After doing a mass update of llvm, and discovering additional issues with NDEBUG, I think it would be better to replace all instances of NDEBUG with the appropriate ${PROJECT}_NDEBUG, and set ${PROJECT})_NDEBUG via cmake (in either config.h or llvm_config.h).

That will break the dependency on NDEBUG, which we don't really control, and allow us to set it on a project basis.

I'm working on a POC patch right now.

[mehdi_amini]

llvm/trunk/CMakeLists.txt

400 ↗

(On Diff #116884)

This solution gets us back into the trouble of picking up the users NDEBUG setting in the header and not necessarily the one used when compiling llvm.

Ah good point!

[MatzeB]

llvm/trunk/CMakeLists.txt

400 ↗

(On Diff #116884)

See also: https://reviews.llvm.org/D11833 and https://reviews.llvm.org/D11834

Would be good to push this topic again.

[hintonda]

llvm/trunk/CMakeLists.txt

400 ↗

(On Diff #116884)

Thanks for the pointer. Yes, I think that's the best solution I've seen.

CMakeLists.txt

 endif()
 option(LLVM_ENABLE_CXX1Y "Compile with C++1y enabled." OFF)
 option(LLVM_ENABLE_CXX1Z "Compile with C++1z enabled." OFF)
 option(LLVM_ENABLE_LIBCXX "Use libc++ if available." OFF)
 option(LLVM_ENABLE_LLD "Use lld as C and C++ linker." OFF)
 option(LLVM_ENABLE_PEDANTIC "Compile with pedantic enabled." ON)
 option(LLVM_ENABLE_WERROR "Fail and stop if a warning is triggered." OFF)
 
+option(LLVM_ENABLE_DUMP "Enable dump functions in release builds" OFF)
+
 if( NOT uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG" )
   option(LLVM_ENABLE_ASSERTIONS "Enable assertions" OFF)
 else()
   option(LLVM_ENABLE_ASSERTIONS "Enable assertions" ON)
 endif()
 
-option(LLVM_ENABLE_EXPENSIVE_CHECKS "Enable expensive checks" OFF)
+if( LLVM_ENABLE_ASSERTIONS )
+  set(LLVM_ENABLE_DUMP ON)
+endif()
 
-option(LLVM_ENABLE_DUMP "Enable dump functions in release builds" OFF)
+option(LLVM_ENABLE_EXPENSIVE_CHECKS "Enable expensive checks" OFF)
 
 set(LLVM_ABI_BREAKING_CHECKS "WITH_ASSERTS" CACHE STRING
   "Enable abi-breaking checks.  Can be WITH_ASSERTS, FORCE_ON or FORCE_OFF.")
 
 option(LLVM_FORCE_USE_OLD_HOST_TOOLCHAIN
        "Set to ON to force using an old, unsupported host toolchain." OFF)
 
 option(LLVM_USE_INTEL_JITEVENTS

cmake/modules/HandleLLVMOptions.cmake

if( LLVM_ENABLE_ASSERTIONS )
   endif()
 endif()
 
 if(LLVM_ENABLE_EXPENSIVE_CHECKS)
   add_definitions(-DEXPENSIVE_CHECKS)
   add_definitions(-D_GLIBCXX_DEBUG)
 endif()
 
-if(LLVM_ENABLE_DUMP)
-  add_definitions(-DLLVM_ENABLE_DUMP)
-endif()
-
 string(TOUPPER "${LLVM_ABI_BREAKING_CHECKS}" uppercase_LLVM_ABI_BREAKING_CHECKS)
 
 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 )

include/llvm/CodeGen/MachineRegisterInfo.h

public:
   MachineInstr *getUniqueVRegDef(unsigned Reg) const;
 
   /// clearKillFlags - Iterate over all the uses of the given register and
   /// clear the kill flag from the MachineOperand. This function is used by
   /// optimization passes which extend register lifetimes and need only
   /// preserve conservative kill flag information.
   void clearKillFlags(unsigned Reg) const;
 
-#ifndef NDEBUG
   void dumpUses(unsigned RegNo) const;
-#endif
 
   /// Returns true if PhysReg is unallocatable and constant throughout the
   /// function. Writing to a constant register has no effect.
   bool isConstantPhysReg(unsigned PhysReg) const;
 
   /// Get an iterator over the pressure sets affected by the given physical or
   /// virtual register. If RegUnit is physical, it must be a register unit (from
   /// MCRegUnitIterator).

include/llvm/CodeGen/MachineScheduler.h

public:
 
   void removeReady(SUnit *SU);
 
   /// Call this before applying any other heuristics to the Available queue.
   /// Updates the Available/Pending Q's if necessary and returns the single
   /// available instruction, or NULL if there are multiple candidates.
   SUnit *pickOnlyChoice();
 
-#ifndef NDEBUG
   void dumpScheduledState() const;
-#endif
 };
 
 /// Base class for GenericScheduler. This class maintains information about
 /// scheduling candidates based on TargetSchedModel making it easy to implement
 /// heuristics for either preRA or postRA scheduling.
 class GenericSchedulerBase : public MachineSchedStrategy {
 public:
   /// Represent the type of SchedCandidate found within a single queue.

include/llvm/CodeGen/TargetSchedule.h

unsigned getNumProcResourceKinds() const {
     return SchedModel.getNumProcResourceKinds();
   }
 
   /// \brief Get a processor resource by ID for convenience.
   const MCProcResourceDesc *getProcResource(unsigned PIdx) const {
     return SchedModel.getProcResource(PIdx);
   }
 
-#ifndef NDEBUG
+#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
   const char *getResourceName(unsigned PIdx) const {
     if (!PIdx)
       return "MOps";
     return SchedModel.getProcResource(PIdx)->Name;
   }
 #endif
 
   using ProcResIter = const MCWriteProcResEntry *;

include/llvm/Config/config.h.cmake

 #cmakedefine01 LLVM_ENABLE_THREADS
 
 /* Define if zlib compression is available */
 #cmakedefine01 LLVM_ENABLE_ZLIB
 
 /* Has gcc/MSVC atomic intrinsics */
 #cmakedefine01 LLVM_HAS_ATOMICS
 
+/* Define if LLVM_ENABLE_DUMP is enabled */
+#cmakedefine LLVM_ENABLE_DUMP
+
 /* Host triple LLVM will be executed on */
 #cmakedefine LLVM_HOST_TRIPLE "${LLVM_HOST_TRIPLE}"
 
 /* Define if this is Unixish platform */
 #cmakedefine LLVM_ON_UNIX ${LLVM_ON_UNIX}
 
 /* Define if this is Win32ish platform */
 #cmakedefine LLVM_ON_WIN32 ${LLVM_ON_WIN32}

include/llvm/MC/MCSchedule.h

 #include <cassert>
 
 namespace llvm {
 
 struct InstrItinerary;
 
 /// Define a kind of processor resource that will be modeled by the scheduler.
 struct MCProcResourceDesc {
-#ifndef NDEBUG
+#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
   const char *Name;
 #endif
   unsigned NumUnits; // Number of resource of this kind
   unsigned SuperIdx; // Index of the resources kind that contains this kind.
 
   // Number of resources that may be buffered.
   //
   // Buffered resources (BufferSize != 0) may be consumed at some indeterminate

 /// Summarize the scheduling resources required for an instruction of a
 /// particular scheduling class.
 ///
 /// Defined as an aggregate struct for creating tables with initializer lists.
 struct MCSchedClassDesc {
   static const unsigned short InvalidNumMicroOps = UINT16_MAX;
   static const unsigned short VariantNumMicroOps = UINT16_MAX - 1;
 
-#ifndef NDEBUG
+#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
   const char* Name;
 #endif
   unsigned short NumMicroOps;
   bool     BeginGroup;
   bool     EndGroup;
   unsigned WriteProcResIdx; // First index into WriteProcResTable.
   unsigned NumWriteProcResEntries;
   unsigned WriteLatencyIdx; // First index into WriteLatencyTable.

lib/CodeGen/LiveDebugVariables.cpp

public:
   /// emitDebugValues - Recreate DBG_VALUE instruction from data structures.
   void emitDebugValues(VirtRegMap *VRM);
 
   void print(raw_ostream&);
 };
 
 } // end anonymous namespace
 
-#ifndef NDEBUG
+#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
 static void printDebugLoc(const DebugLoc &DL, raw_ostream &CommentOS,
                           const LLVMContext &Ctx) {
   if (!DL)
     return;
 
   auto *Scope = cast<DIScope>(DL.getScope());
   // Omit the directory, because it's likely to be long and uninteresting.
   CommentOS << Scope->getFilename();

lib/CodeGen/ScoreboardHazardRecognizer.cpp

 
 #define DEBUG_TYPE DebugType
 
 ScoreboardHazardRecognizer::ScoreboardHazardRecognizer(
     const InstrItineraryData *II, const ScheduleDAG *SchedDAG,
     const char *ParentDebugType)
     : ScheduleHazardRecognizer(), DebugType(ParentDebugType), ItinData(II),
       DAG(SchedDAG) {
+  (void)DebugType;
   // Determine the maximum depth of any itinerary. This determines the depth of
   // the scoreboard. We always make the scoreboard at least 1 cycle deep to
   // avoid dealing with the boundary condition.
   unsigned ScoreboardDepth = 1;
   if (ItinData && !ItinData->isEmpty()) {
     for (unsigned idx = 0; ; ++idx) {
       if (ItinData->isEndMarker(idx))
         break;

utils/TableGen/SubtargetEmitter.cpp

 
 //
 // EmitSchedModel - Emits all scheduling model tables, folding common patterns.
 //
 void SubtargetEmitter::EmitSchedModel(raw_ostream &OS) {
   OS << "#ifdef DBGFIELD\n"
      << "#error \"<target>GenSubtargetInfo.inc requires a DBGFIELD macro\"\n"
      << "#endif\n"
-     << "#ifndef NDEBUG\n"
+     << "#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)\n"
      << "#define DBGFIELD(x) x,\n"
      << "#else\n"
      << "#define DBGFIELD(x)\n"
      << "#endif\n";
 
   if (SchedModels.hasItineraries()) {
     std::vector<std::vector<InstrItinerary>> ProcItinLists;
     // Emit the stage data