[CodeGen] Fixed ambiguous symbol ExtAddrMode in case of NDEBUG and LLVM_ENABLE_DUMP

[slydiman]

Summary

This patch fixes the following error with MSVC 16.9.2 in case of NDEBUG and LLVM_ENABLE_DUMP:
llvm/lib/CodeGen/CodeGenPrepare.cpp(2581): error C2872: 'ExtAddrMode': ambiguous symbol
llvm/include/llvm/CodeGen/TargetInstrInfo.h(86): note: could be 'llvm::ExtAddrMode'
llvm/lib/CodeGen/CodeGenPrepare.cpp(2447): note: or '`anonymous-namespace'::ExtAddrMode'
llvm/lib/CodeGen/CodeGenPrepare.cpp(2581): error C2039: 'print': is not a member of 'llvm::ExtAddrMode'

[barannikov88]

llvm/lib/CodeGen/CodeGenPrepare.cpp

2618

Please refer to the coding style, in particular:

Because of this, we have a simple guideline: make anonymous namespaces as small as possible, and only use them for class declarations.

[slydiman]

llvm/lib/CodeGen/CodeGenPrepare.cpp

2618

It seems the function void ExtAddrMode::print() is related to the struct ExtAddrMode declared above inside the anonymous namespace.
Please correct me if I'm wrong. Do you have any suggestions how to fix this error other way?

[barannikov88]

llvm/lib/CodeGen/CodeGenPrepare.cpp

2618

print is defined under #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP).
It should be declared under the same macros. Same for dump.

As for the error, it seems that it is picking ExtAddrMode from TargetInstrInfo.h somehow (note that it says 'print': is not a member of 'llvm::ExtAddrMode',) though I could not find where this header is included. Are you sure you don't have any local changes that may trigger the issue?

[barannikov88]

llvm/lib/CodeGen/CodeGenPrepare.cpp

2578

It should be left as it was. The intention is to allow printing if either of the macros is defined:
option(LLVM_ENABLE_DUMP "Enable dump functions even when assertions are disabled" OFF)

[slydiman]

llvm/lib/CodeGen/CodeGenPrepare.cpp

2618

I have no any changes. Just defined NDEBUG and LLVM_ENABLE_DUMP.

[barannikov88]

llvm/lib/CodeGen/CodeGenPrepare.cpp

2618

I have no any changes.

According to the description, the compiler complains about line 2562, which does not contain 'printf', so I assumed that you have modified the source (or, at least, have an outdated version).

Just defined NDEBUG and LLVM_ENABLE_DUMP.

Defining both of these macros does not trigger the error for me. But my build OS is Linux.

I have no other ideas what could go wrong here, maybe others do?

[slydiman]

llvm/lib/CodeGen/CodeGenPrepare.cpp

2618

I have updated the summary with more details.

[barannikov88]

llvm/lib/CodeGen/CodeGenPrepare.cpp

2618

So, it is indeed confused with the same-named class from TargetInstrInfo.h. Could you track where it is included?
Adding #error directive to TargetInstrInfo.h and recompiling single CodeGenPrepare.cpp should help to identify this place. For me, however, adding this directive does not prevent the cpp file from being successfully compiled.

[barannikov88]

llvm/lib/CodeGen/CodeGenPrepare.cpp

2618

Calling ::print (with double colon) from the problematic place might workaround the issue.

[barannikov88]

llvm/lib/CodeGen/CodeGenPrepare.cpp

2618

Never mind :)

[aaron.ballman]

llvm/lib/CodeGen/CodeGenPrepare.cpp

2618

+1 to not extending the anonymous namespace like this but instead using a qualified identifier as necessary.

[slydiman]

llvm/lib/CodeGen/CodeGenPrepare.cpp

2618

You do realize that one cannot provide qualified name for unnamed namespace, right? But please feel free to explain if I’m missing something in your comment.
I don’t see how this change possibly violates the developers policy, since this is exactly what is done here - a class declared in an unnamed namespace along with all its memebrs to fix the defect of possible referencing symbols from outside of this CU.
As another approach, we could make that a named namespace, and use qualified names instead. This would also change the symbol visibility/linkage, which I personally do not like.

[barannikov88]

llvm/lib/CodeGen/CodeGenPrepare.cpp

2618

@slydiman
Have you been able to find the place where TargetInstrInfo.h is included? Or is it some MSVC quirk that looks arcoss CUs?

[slydiman]

llvm/lib/CodeGen/CodeGenPrepare.cpp

2618

Unfortunately, I did not find the exact place.
I can reproduce the problem on one of my setups.
It looks like a MSVC quirk around precompiled headers.
As another approach, we could just rename ExtAddrMode to something like ExtTargetLoweringAddrMode keeping the anonymous namespace untouched.

[barannikov88]

llvm/lib/CodeGen/CodeGenPrepare.cpp

2618

Renaming TII's sounds reasonable to me. Something as general as "ExtAddrMode" should not have been added to llvm namespace in the first place (it is not that fundamental class to do so).

Another option is to try to move the class into TargetInstrInfo class declaration.

Either of these option will require changing backends.

If the intention is to just to make it work, renaming the local class seems to be much easier task. Or, alternatively, you could introduce another namespace under the anonymous one. This will keep the class hidden, while disambiguating it with TII's class. You don't see this very often though.

[aaron.ballman]

LGTM!

llvm/lib/CodeGen/CodeGenPrepare.cpp

2618

You do realize that one cannot provide qualified name for unnamed namespace, right? But please feel free to explain if I’m missing something in your comment.

I was thinking we'd be dropping the using namespace llvm; but I realize now, that's going to be fragile and not really fix the underlying issue. Sorry for the noise there!

I'm actually fine with these changes, the alternatives require more invasive changes that I don't think are worth it.

[barannikov88]

llvm/lib/CodeGen/CodeGenPrepare.cpp

2618

I'm actually fine with these changes, the alternatives require more invasive changes that I don't think are worth it.

From that point of view, looks good to me, too.

llvm/lib/CodeGen/CodeGenPrepare.cpp

case ExtAddrMode::BaseOffsField:
       ScaledReg = V;
       Scale = 1;
       BaseOffs = 0;
       break;
     }
   }
 };
 
-} // end anonymous namespace
-
 #ifndef NDEBUG
 static inline raw_ostream &operator<<(raw_ostream &OS, const ExtAddrMode &AM) {
   AM.print(OS);
   return OS;
 }
 #endif
 
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
 void ExtAddrMode::print(raw_ostream &OS) const {
   bool NeedPlus = false;
   OS << "[";
   if (InBounds)
     OS << "inbounds ";
   if (BaseGV) {
     OS << (NeedPlus ? " + " : "")
        << "GV:";

 }
 
 LLVM_DUMP_METHOD void ExtAddrMode::dump() const {
   print(dbgs());
   dbgs() << '\n';
 }
 #endif
 
+} // end anonymous namespace
+
 namespace {
 
 /// This class provides transaction based operation on the IR.
 /// Every change made through this class is recorded in the internal state and
 /// can be undone (rollback) until commit is called.
 /// CGP does not check if instructions could be speculatively executed when
 /// moved. Preserving the original location would pessimize the debugging
 /// experience, as well as negatively impact the quality of sample PGO.