This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Derive from PassInfoMixin for no-op/printing passes
ClosedPublic

Authored by aeubanks on Jul 9 2020, 11:51 AM.

Details

Summary

PassInfoMixin should be used for all NPM passes, rater than a custom
name().

This caused ambiguous references in LegacyPassManager.cpp, so had to
remove "using namespace llvm::legacy" and move some things around.

The passes had to be moved to the llvm namespace, or else they would get
printed as "(anonymous namespace)::FooPass".

Diff Detail

Event Timeline

aeubanks created this revision.Jul 9 2020, 11:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 9 2020, 11:52 AM
ychen added a comment.Jul 9 2020, 12:05 PM

I was aware of this recently. Thanks for fixing this. Just one nit. Please wait for one other reviewer.

llvm/lib/Passes/PassBuilder.cpp
300

How about keeping this local? These are only for testing.

ychen accepted this revision.Jul 9 2020, 12:05 PM
This revision is now accepted and ready to land.Jul 9 2020, 12:05 PM
aeubanks marked an inline comment as done.Jul 9 2020, 12:36 PM
aeubanks added inline comments.
llvm/lib/Passes/PassBuilder.cpp
300

Do you mean keeping this in an anonymous namespace?
As mentioned in the commit, that makes the printed name messed up.

ychen added inline comments.Jul 9 2020, 12:41 PM
llvm/lib/Passes/PassBuilder.cpp
300

Add some regex in lit tests?
Running pass: {{.*}}NoOpModulePass

aeubanks marked an inline comment as done.Jul 9 2020, 1:19 PM
aeubanks added inline comments.
llvm/lib/Passes/PassBuilder.cpp
300

I don't see any reason to distinguish it from other passes, even if it's only used for testing. It's a useful tool for sanity checks. Having a (anonymous namespace) printed anywhere doesn't look good.
And it'd require updating more tests than I really want to.

ychen added inline comments.Jul 9 2020, 4:03 PM
llvm/lib/Passes/PassBuilder.cpp
300

If we really treat them as normal passes, they should be moved to a header file. If we treat them as testing tools only, we put them in .cpp file in an anonymous namespace. It looks confusing to be not in header file and in llvm namespace.

Or perhaps, we don't touch these no-op passes, and add a comment saying we're overriding the name() computing here to make tests cleaner?

aeubanks updated this revision to Diff 276865.Jul 9 2020, 4:22 PM

Put passes/analyses in PassBuilder.cpp back into anonymous namespace, override name

asbirlea accepted this revision.Jul 9 2020, 4:27 PM
asbirlea added inline comments.
llvm/lib/Passes/PassBuilder.cpp
300

IMO a comment clarifying these are a special case will work here.

aeubanks updated this revision to Diff 276872.Jul 9 2020, 4:49 PM

Update comment

This revision was automatically updated to reflect the committed changes.
davide added a subscriber: davide.Jul 10 2020, 11:17 AM

This broke the modules build on macOS.

warning: /Applications/Xcode5.app/Contents/Developer/Toolchains/OSX10.15.xctoolchain/usr/bin/libtool: warning for library: lib/libLLVMExtensions.a the table of contents is empty (no object file members in the library define global symbols)
[320/3939] Building CXX object lib/Remarks/CMakeFiles/LLVMRemarks.dir/RemarkLinker.cpp.o
FAILED: lib/Remarks/CMakeFiles/LLVMRemarks.dir/RemarkLinker.cpp.o 
/Applications/Xcode5.app/Contents/Developer/Toolchains/OSX10.15.xctoolchain/usr/bin/c++  -DGTEST_HAS_RTTI=0 -D_DEBUG -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Ilib/Remarks -I/Users/davide/work/llvm-project/llvm/lib/Remarks -I/Applications/Xcode5.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.Internal.sdk/usr/include/libxml2 -Iinclude -I/Users/davide/work/llvm-project/llvm/include -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -fmodules -fmodules-cache-path=/Users/davide/work/build-modules/module.cache -fcxx-modules -Xclang -fmodules-local-submodule-visibility -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -O3  -isysroot /Applications/Xcode5.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.Internal.sdk    -fno-exceptions -fno-rtti -UNDEBUG -std=c++14 -MD -MT lib/Remarks/CMakeFiles/LLVMRemarks.dir/RemarkLinker.cpp.o -MF lib/Remarks/CMakeFiles/LLVMRemarks.dir/RemarkLinker.cpp.o.d -o lib/Remarks/CMakeFiles/LLVMRemarks.dir/RemarkLinker.cpp.o -c /Users/davide/work/llvm-project/llvm/lib/Remarks/RemarkLinker.cpp
While building module 'LLVM_Object' imported from /Users/davide/work/llvm-project/llvm/include/llvm/Remarks/RemarkLinker.h:16:
While building module 'LLVM_IR' imported from /Users/davide/work/llvm-project/llvm/include/llvm/Object/IRSymtab.h:29:
While building module 'LLVM_intrinsic_gen' imported from /Users/davide/work/llvm-project/llvm/include/llvm/IR/IRPrintingPasses.h:22:
In file included from <module-includes>:1:
In file included from /Users/davide/work/llvm-project/llvm/include/llvm/IR/Argument.h:18:
/Users/davide/work/llvm-project/llvm/include/llvm/IR/Attributes.h:75:14: fatal error: 'llvm/IR/Attributes.inc' file not found
    #include "llvm/IR/Attributes.inc"
             ^~~~~~~~~~~~~~~~~~~~~~~~
While building module 'LLVM_Object' imported from /Users/davide/work/llvm-project/llvm/include/llvm/Remarks/RemarkLinker.h:16:
While building module 'LLVM_IR' imported from /Users/davide/work/llvm-project/llvm/include/llvm/Object/IRSymtab.h:29:
In file included from <module-includes>:4:
/Users/davide/work/llvm-project/llvm/include/llvm/IR/IRPrintingPasses.h:22:10: fatal error: could not build module 'LLVM_intrinsic_gen'
#include "llvm/IR/PassManager.h"
 ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
While building module 'LLVM_Object' imported from /Users/davide/work/llvm-project/llvm/include/llvm/Remarks/RemarkLinker.h:16:
In file included from <module-includes>:4:
/Users/davide/work/llvm-project/llvm/include/llvm/Object/IRSymtab.h:29:10: fatal error: could not build module 'LLVM_IR'
#include "llvm/IR/GlobalValue.h"
 ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
In file included from /Users/davide/work/llvm-project/llvm/lib/Remarks/RemarkLinker.cpp:13:
/Users/davide/work/llvm-project/llvm/include/llvm/Remarks/RemarkLinker.h:16:10: fatal error: could not build module 'LLVM_Object'
#include "llvm/Object/ObjectFile.h"
 ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~
4 errors generated.

I'm going to revert, and I'm going to follow up with precise instructions on how to repro.

on any recent'ish macOS (although, I don't think the OS quite matters)

% xcrun cmake -GNinja -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_MODULES=On -DLLVM_ENABLE_ASSERTIONS:BOOL=TRUE -DLLDB_ENABLE_PYTHON=On

then

% ninja check-lldb

I feel the only relevant bit is -DLLVM_ENABLE_MODULES=On , YMMV.

Reverted in:

commit fdb7856d54a1f81bab0ac0c8a4e984620589e699 (HEAD -> master, origin/master, origin/HEAD)
Author: Davide Italiano <ditaliano@apple.com>
Date:   Fri Jul 10 11:16:33 2020 -0700

    Revert "[NFC] Derive from PassInfoMixin for no-op/printing passes"
    
    This reverts commit 8039d2c3bf14585ef37dc9343bf393ecad9aead9 as
    it breaks the modules build on macOS.

Don't hesitate to ping me if you need any other info to reproduce.

Reverted in:

commit fdb7856d54a1f81bab0ac0c8a4e984620589e699 (HEAD -> master, origin/master, origin/HEAD)
Author: Davide Italiano <ditaliano@apple.com>
Date:   Fri Jul 10 11:16:33 2020 -0700

    Revert "[NFC] Derive from PassInfoMixin for no-op/printing passes"
    
    This reverts commit 8039d2c3bf14585ef37dc9343bf393ecad9aead9 as
    it breaks the modules build on macOS.

Don't hesitate to ping me if you need any other info to reproduce.

Repro'ed, fixed (hopefully), and relanded in 21b4cc1db9f4eb6d6956802257e3a80f86045c67.