This is an archive of the discontinued LLVM Phabricator instance.

Remove "Rewrite Symbols" from codegen pipeline
ClosedPublic

Authored by aeubanks on Mar 31 2021, 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.

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

addr-label.ll crashes on ARM due to this change. This is because a
ARMConstantPoolConstant containing a BasicBlock to represent a
blockaddress may hold an invalid pointer to a BasicBlock if the
blockaddress is invalidated by its BasicBlock getting removed. In that
case all referencing blockaddresses are RAUW a constant int. Making
ARMConstantPoolConstant::CVal a WeakVH fixes the crash, but I'm not sure
that's the right fix. As a workaround, create a barrier right before
ISel so that IR optimizations can't happen while a
ARMConstantPoolConstant has been created.

Diff Detail

Event Timeline

aeubanks created this revision.Mar 31 2021, 11:26 PM
aeubanks requested review of this revision.Mar 31 2021, 11:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2021, 11:26 PM
rnk added a subscriber: compnerd.Apr 1 2021, 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.Apr 5 2021, 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.Apr 5 2021, 5:59 PM

add test for only one function pass manager

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

I'll submit the revert separately when landing

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

lgtm, thanks

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

rebase after revert

This revision was automatically updated to reflect the committed changes.
aeubanks reopened this revision.May 3 2021, 9:25 PM
This revision is now accepted and ready to land.May 3 2021, 9:25 PM
aeubanks updated this revision to Diff 342632.May 3 2021, 9:25 PM

rebase and handle ARM

adding some ARM people to look at the ARMTargetMachine change

aeubanks edited the summary of this revision. (Show Details)May 3 2021, 9:28 PM
SjoerdMeijer added inline comments.
llvm/lib/Target/ARM/ARMTargetMachine.cpp
466 ↗(On Diff #342632)

Thanks for adding us. I am unfamiliar with this BarriesNoopPass. @dmgreen, do you know more about this by any chance?

aeubanks added inline comments.May 4 2021, 10:27 AM
llvm/lib/Target/ARM/ARMTargetMachine.cpp
466 ↗(On Diff #342632)

typically if we have a bunch of function passes in a row, the pass manager will run all of them on one function, then the next function, etc
by adding a module pass somewhere in the middle of the function passes, we'll end up running the first half of the function passes on each function, then the second half. this accomplishes what's mentioned in the description

e.g. if we have two functions f1 and f2, and four function passes FP1-4, normally it'd be
FP1 on f1
FP2 on f1
FP3 on f1
FP4 on f1
FP1 on f2
FP2 on f2
FP3 on f2
FP4 on f2

but with the barrier between FP2 and FP3, we get

FP1 on f1
FP2 on f1
FP1 on f2
FP2 on f2
barrier
FP3 on f1
FP4 on f1
FP3 on f2
FP4 on f2

rnk added inline comments.May 4 2021, 10:39 AM
llvm/lib/Target/ARM/ARMTargetMachine.cpp
466 ↗(On Diff #342632)

I think this is a reasonable temporary solution, but please add a comment about what this is doing.

The module pass barrier ensures that we run all pre-ISel passes (IR passes) over all functions before starting instruction selection. IR passes can modify the CFG, which can delete address-taken basic blocks. That invalidates references to block addresses stored in the ARM constant pool. Maybe add // FIXME: Solve this another way or something like that.

aeubanks updated this revision to Diff 342807.May 4 2021, 11:12 AM

add FIXME

rnk accepted this revision.May 4 2021, 11:17 AM

Looks good to me, but let's wait for an ARM backend person to acknowledge the ARM change.

compnerd accepted this revision.May 12 2021, 11:19 AM

ping @ an ARM owner

llvm/lib/Target/ARM/ARMTargetMachine.cpp
466 ↗(On Diff #342632)
rnk added a comment.May 27 2021, 2:37 PM

In the absence of review from ARM, I think we should land this as is.

Sorry for the delay. Looks reasonable to me.

This revision was landed with ongoing or failed builds.May 31 2021, 8:32 AM
This revision was automatically updated to reflect the committed changes.