Remove "Rewrite Symbols" from codegen pipeline

[aeubanks]

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.

[rnk]

+@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.

[aeubanks]

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...

[MaskRay]

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]

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

[aeubanks]

add test for only one function pass manager

[aeubanks]

I'll submit the revert separately when landing

[rnk]

lgtm, thanks

[MaskRay]

@compnerd ^^

[aeubanks]

rebase after revert

[aeubanks]

rebase and handle ARM

[aeubanks]

adding some ARM people to look at the ARMTargetMachine change

[SjoerdMeijer]

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]

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]

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]

add FIXME

[rnk]

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

[aeubanks]

ping

[MaskRay]

Ping @compnerd

[aeubanks]

ping @ an ARM owner

llvm/lib/Target/ARM/ARMTargetMachine.cpp

466 ↗

(On Diff #342632)

ping @SjoerdMeijer @dmgreen

[rnk]

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

[SjoerdMeijer]

Sorry for the delay. Looks reasonable to me.

llvm/include/llvm/CodeGen/MachineModuleInfo.h

MCSymbol *getAddrLabelSymbol(const BasicBlock *BB) {
     return getAddrLabelSymbolToEmit(BB).front();
   }
 
   /// Return the symbol to be used for the specified basic block when its
   /// address is taken.  If other blocks were RAUW'd to this one, we may have
   /// to emit them as well, return the whole set.
   ArrayRef<MCSymbol *> getAddrLabelSymbolToEmit(const BasicBlock *BB);
 
+  /// If the specified function has had any references to address-taken blocks
+  /// generated, but the block got deleted, return the symbol now so we can
+  /// emit it.  This prevents emitting a reference to a symbol that has no
+  /// definition.
+  void takeDeletedSymbolsForFunction(const Function *F,
+                                     std::vector<MCSymbol *> &Result);
+
   /// \name Exception Handling
   /// \{
 
   /// Set the call site currently being processed.
   void setCurrentCallSite(unsigned Site) { CurCallSite = Site; }
 
   /// Get the call site currently being processed, if any.  return zero if
   /// none.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

void AsmPrinter::emitFunctionHeader() {
   // descriptors and should be converted to use this hook as well.
   if (MAI->needsFunctionDescriptors())
     emitFunctionDescriptor();
 
   // Emit the CurrentFnSym. This is a virtual function to allow targets to do
   // their wild and crazy things as required.
   emitFunctionEntryLabel();
 
+  // If the function had address-taken blocks that got deleted, then we have
+  // references to the dangling symbols.  Emit them at the start of the function
+  // so that we don't get references to undefined symbols.
+  std::vector<MCSymbol *> DeadBlockSyms;
+  MMI->takeDeletedSymbolsForFunction(&F, DeadBlockSyms);
+  for (unsigned i = 0, e = DeadBlockSyms.size(); i != e; ++i) {
+    OutStreamer->AddComment("Address taken block that was later removed");
+    OutStreamer->emitLabel(DeadBlockSyms[i]);
+  }
+
   if (CurrentFnBegin) {
     if (MAI->useAssignmentForEHBegin()) {
       MCSymbol *CurPos = OutContext.createTempSymbol();
       OutStreamer->emitLabel(CurPos);
       OutStreamer->emitAssignment(CurrentFnBegin,
                                  MCSymbolRefExpr::create(CurPos, OutContext));
     } else {
       OutStreamer->emitLabel(CurrentFnBegin);

llvm/lib/CodeGen/MachineModuleInfo.cpp

class MMIAddrLabelMap {
   };
 
   DenseMap<AssertingVH<BasicBlock>, AddrLabelSymEntry> AddrLabelSymbols;
 
   /// Callbacks for the BasicBlock's that we have entries for.  We use this so
   /// we get notified if a block is deleted or RAUWd.
   std::vector<MMIAddrLabelMapCallbackPtr> BBCallbacks;
 
+  /// This is a per-function list of symbols whose corresponding BasicBlock got
+  /// deleted.  These symbols need to be emitted at some point in the file, so
+  /// AsmPrinter emits them after the function body.
+  DenseMap<AssertingVH<Function>, std::vector<MCSymbol *>>
+      DeletedAddrLabelsNeedingEmission;
+
 public:
   MMIAddrLabelMap(MCContext &context) : Context(context) {}
 
+  ~MMIAddrLabelMap() {
+    assert(DeletedAddrLabelsNeedingEmission.empty() &&
+           "Some labels for deleted blocks never got emitted");
+  }
+
   ArrayRef<MCSymbol *> getAddrLabelSymbolToEmit(BasicBlock *BB);
 
+  void takeDeletedSymbolsForFunction(Function *F,
+                                     std::vector<MCSymbol *> &Result);
+
   void UpdateForDeletedBlock(BasicBlock *BB);
   void UpdateForRAUWBlock(BasicBlock *Old, BasicBlock *New);
 };
 
 } // end namespace llvm
 
 ArrayRef<MCSymbol *> MMIAddrLabelMap::getAddrLabelSymbolToEmit(BasicBlock *BB) {
   assert(BB->hasAddressTaken() &&
ArrayRef<MCSymbol *> MMIAddrLabelMap::getAddrLabelSymbolToEmit(BasicBlock *BB) {
   Entry.Index = BBCallbacks.size() - 1;
   Entry.Fn = BB->getParent();
   MCSymbol *Sym = BB->hasAddressTaken() ? Context.createNamedTempSymbol()
                                         : Context.createTempSymbol();
   Entry.Symbols.push_back(Sym);
   return Entry.Symbols;
 }
 
+/// If we have any deleted symbols for F, return them.
+void MMIAddrLabelMap::takeDeletedSymbolsForFunction(
+    Function *F, std::vector<MCSymbol *> &Result) {
+  DenseMap<AssertingVH<Function>, std::vector<MCSymbol *>>::iterator I =
+      DeletedAddrLabelsNeedingEmission.find(F);
+
+  // If there are no entries for the function, just return.
+  if (I == DeletedAddrLabelsNeedingEmission.end())
+    return;
+
+  // Otherwise, take the list.
+  std::swap(Result, I->second);
+  DeletedAddrLabelsNeedingEmission.erase(I);
+}
+
 void MMIAddrLabelMap::UpdateForDeletedBlock(BasicBlock *BB) {
   // If the block got deleted, there is no need for the symbol.  If the symbol
   // was already emitted, we can just forget about it, otherwise we need to
   // queue it up for later emission when the function is output.
   AddrLabelSymEntry Entry = std::move(AddrLabelSymbols[BB]);
   AddrLabelSymbols.erase(BB);
   assert(!Entry.Symbols.empty() && "Didn't have a symbol, why a callback?");
   BBCallbacks[Entry.Index] = nullptr;  // Clear the callback.
 
   assert((BB->getParent() == nullptr || BB->getParent() == Entry.Fn) &&
          "Block/parent mismatch");
 
-  assert(llvm::all_of(Entry.Symbols, [](MCSymbol *Sym) {
-    return Sym->isDefined(); }));
+  for (MCSymbol *Sym : Entry.Symbols) {
+    if (Sym->isDefined())
+      return;
+
+    // If the block is not yet defined, we need to emit it at the end of the
+    // function.  Add the symbol to the DeletedAddrLabelsNeedingEmission list
+    // for the containing Function.  Since the block is being deleted, its
+    // parent may already be removed, we have to get the function from 'Entry'.
+    DeletedAddrLabelsNeedingEmission[Entry.Fn].push_back(Sym);
+  }
 }
 
 void MMIAddrLabelMap::UpdateForRAUWBlock(BasicBlock *Old, BasicBlock *New) {
   // Get the entry for the RAUW'd block and remove it from our map.
   AddrLabelSymEntry OldEntry = std::move(AddrLabelSymbols[Old]);
   AddrLabelSymbols.erase(Old);
   assert(!OldEntry.Symbols.empty() && "Didn't have a symbol, why a callback?");
 

 ArrayRef<MCSymbol *>
 MachineModuleInfo::getAddrLabelSymbolToEmit(const BasicBlock *BB) {
   // Lazily create AddrLabelSymbols.
   if (!AddrLabelSymbols)
     AddrLabelSymbols = new MMIAddrLabelMap(getContext());
  return AddrLabelSymbols->getAddrLabelSymbolToEmit(const_cast<BasicBlock*>(BB));
 }
 
+void MachineModuleInfo::takeDeletedSymbolsForFunction(
+    const Function *F, std::vector<MCSymbol *> &Result) {
+  // If no blocks have had their addresses taken, we're done.
+  if (!AddrLabelSymbols)
+    return;
+  return AddrLabelSymbols->takeDeletedSymbolsForFunction(
+      const_cast<Function *>(F), Result);
+}
+
 /// \name Exception Handling
 /// \{
 
 void MachineModuleInfo::addPersonality(const Function *Personality) {
   if (!llvm::is_contained(Personalities, Personality))
     Personalities.push_back(Personality);
 }
 

llvm/lib/CodeGen/TargetPassConfig.cpp

void TargetPassConfig::addPassesToHandleExceptions() {
   }
 }
 
 /// Add pass to prepare the LLVM IR for code generation. This should be done
 /// before exception handling preparation passes.
 void TargetPassConfig::addCodeGenPrepare() {
   if (getOptLevel() != CodeGenOpt::None && !DisableCGP)
     addPass(createCodeGenPreparePass());
-  addPass(createRewriteSymbolsPass());
 }
 
 /// Add common passes that perform LLVM IR to IR transforms in preparation for
 /// instruction selection.
 void TargetPassConfig::addISelPrepare() {
   addPreISel();
 
   // Force codegen to run according to the callgraph.

llvm/test/CodeGen/AArch64/O0-pipeline.ll

 ; CHECK-NEXT:       Module Verifier
 ; CHECK-NEXT:       Lower Garbage Collection Instructions
 ; CHECK-NEXT:       Shadow Stack GC Lowering
 ; CHECK-NEXT:       Lower constant intrinsics
 ; CHECK-NEXT:       Remove unreachable blocks from the CFG
 ; CHECK-NEXT:       Scalarize Masked Memory Intrinsics
 ; CHECK-NEXT:       Expand reduction intrinsics
 ; CHECK-NEXT:       AArch64 Stack Tagging
-; CHECK-NEXT:     Rewrite Symbols
-; CHECK-NEXT:     FunctionPass Manager
 ; CHECK-NEXT:       Exception handling preparation
 ; CHECK-NEXT:       Safe Stack instrumentation pass
 ; CHECK-NEXT:       Insert stack protectors
 ; CHECK-NEXT:       Module Verifier
 ; CHECK-NEXT:       Analysis containing CSE Info
 ; CHECK-NEXT:       IRTranslator
 ; CHECK-NEXT:       Analysis for ComputingKnownBits
 ; CHECK-NEXT:       Analysis containing CSE Info

llvm/test/CodeGen/AArch64/O3-pipeline.ll

 ; CHECK-NEXT:       AArch64 Stack Tagging
 ; CHECK-NEXT:       Function Alias Analysis Results
 ; CHECK-NEXT:       Memory SSA
 ; CHECK-NEXT:       Interleaved Load Combine Pass
 ; CHECK-NEXT:       Dominator Tree Construction
 ; CHECK-NEXT:       Interleaved Access Pass
 ; CHECK-NEXT:       Natural Loop Information
 ; CHECK-NEXT:       CodeGen Prepare
-; CHECK-NEXT:     Rewrite Symbols
-; CHECK-NEXT:     FunctionPass Manager
 ; CHECK-NEXT:       Dominator Tree Construction
 ; CHECK-NEXT:       Exception handling preparation
 ; CHECK-NEXT:     AArch64 Promote Constant
 ; CHECK-NEXT:       FunctionPass Manager
 ; CHECK-NEXT:         Dominator Tree Construction
 ; CHECK-NEXT:     FunctionPass Manager
 ; CHECK-NEXT:       Merge internal globals
 ; CHECK-NEXT:       Safe Stack instrumentation pass

llvm/test/CodeGen/ARM/O3-pipeline.ll

 ; CHECK-NEXT:      Basic Alias Analysis (stateless AA impl)
 ; CHECK-NEXT:      Function Alias Analysis Results
 ; CHECK-NEXT:      Transform functions to use DSP intrinsics
 ; CHECK-NEXT:      Interleaved Access Pass
 ; CHECK-NEXT:      Type Promotion
 ; CHECK-NEXT:      Dominator Tree Construction
 ; CHECK-NEXT:      Natural Loop Information
 ; CHECK-NEXT:      CodeGen Prepare
-; CHECK-NEXT:    Rewrite Symbols
-; CHECK-NEXT:    FunctionPass Manager
 ; CHECK-NEXT:      Dominator Tree Construction
 ; CHECK-NEXT:      Exception handling preparation
 ; CHECK-NEXT:      Merge internal globals
 ; CHECK-NEXT:      Natural Loop Information
 ; CHECK-NEXT:      Scalar Evolution Analysis
 ; CHECK-NEXT:      Lazy Branch Probability Analysis
 ; CHECK-NEXT:      Lazy Block Frequency Analysis
 ; CHECK-NEXT:      Optimization Remark Emitter

llvm/test/CodeGen/Generic/addr-label.ll

entry:
   ret i32 -1
 test_label:
   br label %ret
 ret:
   ret i32 -1
 }
 
 
-;; Issues with referring to a label that gets RAUW'd later.
+; Issues with referring to a label that gets RAUW'd later.
 define i32 @test2a() nounwind {
 entry:
         %target = bitcast i8* blockaddress(@test2b, %test_label) to i8*
 
         call i32 @test2b(i8* %target)
 
         ret i32 0
 }

llvm/test/CodeGen/X86/O0-pipeline.ll

 ; CHECK-NEXT:       Module Verifier
 ; CHECK-NEXT:       Lower Garbage Collection Instructions
 ; CHECK-NEXT:       Shadow Stack GC Lowering
 ; CHECK-NEXT:       Lower constant intrinsics
 ; CHECK-NEXT:       Remove unreachable blocks from the CFG
 ; CHECK-NEXT:       Scalarize Masked Memory Intrinsics
 ; CHECK-NEXT:       Expand reduction intrinsics
 ; CHECK-NEXT:       Expand indirectbr instructions
-; CHECK-NEXT:     Rewrite Symbols
-; CHECK-NEXT:     FunctionPass Manager
 ; CHECK-NEXT:       Exception handling preparation
 ; CHECK-NEXT:       Safe Stack instrumentation pass
 ; CHECK-NEXT:       Insert stack protectors
 ; CHECK-NEXT:       Module Verifier
 ; CHECK-NEXT:       X86 DAG->DAG Instruction Selection
 ; CHECK-NEXT:       X86 PIC Global Base Reg Initialization
 ; CHECK-NEXT:       Finalize ISel and expand pseudo-instructions
 ; CHECK-NEXT:       Local Stack Slot Allocation

llvm/test/CodeGen/X86/opt-pipeline.ll

 ; CHECK-NEXT:       Partially inline calls to library functions
 ; CHECK-NEXT:       Scalarize Masked Memory Intrinsics
 ; CHECK-NEXT:       Expand reduction intrinsics
 ; CHECK-NEXT:       Interleaved Access Pass
 ; CHECK-NEXT:       X86 Partial Reduction
 ; CHECK-NEXT:       Expand indirectbr instructions
 ; CHECK-NEXT:       Natural Loop Information
 ; CHECK-NEXT:       CodeGen Prepare
-; CHECK-NEXT:     Rewrite Symbols
-; CHECK-NEXT:     FunctionPass Manager
 ; CHECK-NEXT:       Dominator Tree Construction
 ; CHECK-NEXT:       Exception handling preparation
 ; CHECK-NEXT:       Safe Stack instrumentation pass
 ; CHECK-NEXT:       Insert stack protectors
 ; CHECK-NEXT:       Module Verifier
 ; CHECK-NEXT:       Basic Alias Analysis (stateless AA impl)
 ; CHECK-NEXT:       Function Alias Analysis Results
 ; CHECK-NEXT:       Natural Loop Information

llvm/test/CodeGen/X86/select_meta.ll

-; RUN: llc -mtriple=x86_64-unknown-unknown -print-after-all < %s 2>&1 | FileCheck %s
+; RUN: opt -mtriple=x86_64-unknown-unknown -codegenprepare -S < %s 2>&1 | FileCheck %s
 
 ; Function Attrs: norecurse nounwind readnone uwtable
 define i32 @foo(i32, i32, i32) {
   %4 = and i32 %0, 3
   %5 = icmp eq i32 %4, 1
   %6 = select i1 %5, i32 %1, i32 %2, !prof  !1
 ; CHECK: br {{.*}}label{{.*}}, label{{.*}}, !prof ![[WT:.*]]
   ret i32 %6
 }
 
 !llvm.ident = !{!0}
 
 !0 = !{!"clang version 4.0.0 (trunk 279683)"}
 !1 = !{!"branch_weights", i32 1000, i32 1 }
 ; CHECK: ![[WT]] = !{!"branch_weights", i32 1000, i32 1}

llvm/test/Other/2010-05-06-Printer.ll

 ; RUN: llc -O2 -print-after-all < %s 2>/dev/null
 ; RUN: llc -O2 -print-after-all < %s 2>&1 | FileCheck %s --check-prefix=ALL
 ; RUN: llc -O2 -print-after-all -filter-print-funcs=foo < %s 2>&1 | FileCheck %s --check-prefix=FOO
 ; REQUIRES: default_triple
 define void @tester(){
   ret void
 }
 
 define void @foo(){
   ret void
 }
 
-;ALL-NOT: IR Dump After {{.*}}; ModuleID =
 ;ALL: define void @tester()
 ;ALL: define void @foo()
-;ALL: ModuleID =
 
 ;FOO: IR Dump After
 ;FOO-NEXT: define void @foo()
 ;FOO-NOT: define void @tester