Page MenuHomePhabricator

Remove "Rewrite Symbols" from codegen pipeline
ClosedPublic

Authored by aeubanks on Wed, Mar 31, 11:26 PM.

Details

Summary

It breaks up the function pass manager in the codegen pipeline.

With empty parameters, it looks at the -mllvm flag -rewrite-map-file.
This is likely not in use.

Add a check that we only have one function pass manager in the codegen
pipeline.

This also reverts commit 9583a3f2625818b78c0cf6d473cdedb9f23ad82c:
"[AsmPrinter] Delete dead takeDeletedSymbsForFunction()".
This was not NFC as initially thought. By coalescing two function
psas managers, this exposed the reverted code as necessary.
addr-label.ll was crashing due to an emitted blockaddress's block being
removed but the label not emitted.

Some tests relied on the fact that we had a module pass somewhere in the
codegen pipeline.

Diff Detail

Event Timeline

aeubanks created this revision.Wed, Mar 31, 11:26 PM
aeubanks requested review of this revision.Wed, Mar 31, 11:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Mar 31, 11:26 PM
rnk added a subscriber: compnerd.Thu, Apr 1, 11:10 AM

+@compnerd

With empty parameters, it's a no-op and also breaks up the function pass manager.

I'm not sure it's actually a no-op, see the constructor which parses a cl::opt provided file:
https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/Transforms/Utils/SymbolRewriter.h#L122

RewriteSymbolPass() { loadAndParseMapFiles(); }

To preserve the functionality, we would need to make adding the pass conditional on the cl::opt being empty. OTOH, I'm not convinced that anyone is actually using this functionality. The top search hits for the command line flag are the Doxygen for the source files.

When this lands, please split it into a straight revert of the function symbol patch the pipeline adjustment change.

Yeah it's not technically no-op, but that flag should only be used for testing.
There is a clang flag to enable this pass in the optimization pipeline: -frewrite-map-file. Although actually it's not implemented for the new PM...

I think people who need symbol rename are mostly using #pragma redefine_extname or asm labels... (others are using objcopy --redefine-sym/--redefine-syms)

aeubanks planned changes to this revision.Mon, Apr 5, 3:00 PM

Will update the commit message and add a check that we only have one function pass manager in the codegen pipeline

aeubanks updated this revision to Diff 335361.Mon, Apr 5, 5:59 PM

add test for only one function pass manager

aeubanks edited the summary of this revision. (Show Details)Mon, Apr 5, 5:59 PM

I'll submit the revert separately when landing

rnk accepted this revision.Wed, Apr 7, 11:02 AM

lgtm, thanks

This revision is now accepted and ready to land.Wed, Apr 7, 11:02 AM
aeubanks updated this revision to Diff 335888.Wed, Apr 7, 11:48 AM

rebase after revert

This revision was automatically updated to reflect the committed changes.