Removed skip and moved processInstructionForFuncReferences, nits

Author: Alexander Yermolovich

bolt/include/bolt/Core/BinaryContext.h

@@ -1350,9 +1350,6 @@ class BinaryContext {
     return Code.size();
   }
 
-  /// Processes .text section to identify function references.
-  void processInstructionForFuncReferences(const MCInst &Inst);
-
   /// Compute the native code size for a range of instructions.
   /// Note: this can be imprecise wrt the final binary since happening prior to
   /// relaxation, as well as wrt the original binary because of opcode

bolt/include/bolt/Core/BinaryFunction.h

@@ -428,7 +428,7 @@ class BinaryFunction {
   /// Function order for streaming into the destination binary.
   uint32_t Index{-1U};
 
-  /// Indicates function is referenced by none-control flow instruction.
+  /// Indicates function is referenced by non-control flow instruction.
   bool HasAddressTaken{false};
 
   /// Get basic block index assuming it belongs to this function.
@@ -820,10 +820,10 @@ class BinaryFunction {
     return nullptr;
   }
 
-  /// Returns true if function is referenced in none-control flow instructions..
+  /// Return true if function is referenced in non-control flow instructions..
   bool hasAddressTaken() const { return HasAddressTaken; }
 
-  /// Sets whether function is referenced in none-control flow instructions.
+  /// Set whether function is referenced in non-control flow instructions.
   void setHasAddressTaken(bool Hat) { HasAddressTaken = Hat; }
 
   /// Returns the raw binary encoding of this function.
@@ -2103,6 +2103,10 @@ class BinaryFunction {
   // adjustments.
   void handleAArch64IndirectCall(MCInst &Instruction, const uint64_t Offset);
 
+  /// Processes code section to identify function references.
+  void processInstructionForFuncReferences(BinaryContext &BC,
+                                           const MCInst &Inst);
+
   /// Scan function for references to other functions. In relocation mode,
   /// add relocations for external references. In non-relocation mode, detect
   /// and mark new entry points.

bolt/lib/Core/BinaryContext.cpp

@@ -1945,28 +1945,6 @@ static void printDebugInfo(raw_ostream &OS, const MCInst &Instruction,
     OS << " discriminator:" << Row.Discriminator;
 }
 
-/// Skip instructions that do not potentially manipulate or compare function
-/// addresses.
-static bool skipInstruction(const MCInst &Inst, const BinaryContext &BC) {
-  return (BC.MIB->isPseudo(Inst) || BC.MIB->isUnconditionalBranch(Inst) ||
-          BC.MIB->isConditionalBranch(Inst) || BC.MIB->isCall(Inst) ||
-          BC.MIB->isBranch(Inst));
-}
-void BinaryContext::processInstructionForFuncReferences(const MCInst &Inst) {
-  if (skipInstruction(Inst, *this))
-    return;
-  for (const MCOperand &Op : MCPlus::primeOperands(Inst)) {
-    if (!Op.isExpr())
-      continue;
-    const MCExpr &Expr = *Op.getExpr();
-    if (Expr.getKind() == MCExpr::SymbolRef) {
-      const MCSymbol &Symbol = cast<MCSymbolRefExpr>(Expr).getSymbol();
-      if (BinaryFunction *BF = getFunctionForSymbol(&Symbol))
-        BF->setHasAddressTaken(true);
-    }
-  }
-}
-
 void BinaryContext::printInstruction(raw_ostream &OS, const MCInst &Instruction,
                                      uint64_t Offset,
                                      const BinaryFunction *Function,

bolt/lib/Core/BinaryFunction.cpp

@@ -1513,6 +1513,20 @@ MCSymbol *BinaryFunction::registerBranch(uint64_t Src, uint64_t Dst) {
   return Target;
 }
 
+void BinaryFunction::processInstructionForFuncReferences(BinaryContext &BC,
+                                                         const MCInst &Inst) {
+  for (const MCOperand &Op : MCPlus::primeOperands(Inst)) {
+    if (!Op.isExpr())
+      continue;
+    const MCExpr &Expr = *Op.getExpr();
+    if (Expr.getKind() == MCExpr::SymbolRef) {
+      const MCSymbol &Symbol = cast<MCSymbolRefExpr>(Expr).getSymbol();
+      if (BinaryFunction *BF = BC.getFunctionForSymbol(&Symbol))
+        BF->setHasAddressTaken(true);
+    }
+  }
+}
+
 bool BinaryFunction::scanExternalRefs() {
   bool Success = true;
   bool DisassemblyFailed = false;
@@ -1626,15 +1640,15 @@ bool BinaryFunction::scanExternalRefs() {
     if (!BC.HasRelocations)
       continue;
 
-    BC.processInstructionForFuncReferences(Instruction);
-
     if (BranchTargetSymbol) {
       BC.MIB->replaceBranchTarget(Instruction, BranchTargetSymbol,
                                   Emitter.LocalCtx.get());
     } else if (!llvm::any_of(Instruction,
                              [](const MCOperand &Op) { return Op.isExpr(); })) {
       // Skip assembly if the instruction may not have any symbolic operands.
       continue;
+    } else {
+      processInstructionForFuncReferences(BC, Instruction);
     }
 
     // Emit the instruction using temp emitter and generate relocations.

bolt/lib/Passes/IdenticalCodeFolding.cpp

@@ -439,9 +439,12 @@ Error IdenticalCodeFolding::markFunctionsUnsafeToFold(BinaryContext &BC) {
   }
 
   ParallelUtilities::WorkFuncTy WorkFun = [&](BinaryFunction &BF) {
-    for (const BinaryBasicBlock *BB : BF.getLayout().blocks())
-      for (const MCInst &Inst : *BB)
-        BC.processInstructionForFuncReferences(Inst);
+    for (const BinaryBasicBlock *BB : BF.getLayout().blocks()) {
+      for (const MCInst &Inst : *BB) {
+        if (!(BC.MIB->isCall(Inst) || BC.MIB->isBranch(Inst)))
+          BF.processInstructionForFuncReferences(BC, Inst);
+      }
+    }
   };
   ParallelUtilities::PredicateTy SkipFunc =
       [&](const BinaryFunction &BF) -> bool {

bolt/test/X86/icf-safe-icp.test

@@ -1,5 +1,6 @@
-## Check that BOLT handles correctly folding functions with --icf=safe that can be referenced.
-## The compare is generated by the ICP path with instrumentation profiling.
+## Check that BOLT handles correctly folding functions with --icf=safe
+## that can be referenced through a non control flow instruction when ICP optimization is enabled.
+## This tests also checks that destructors are not folded.
 
 # REQUIRES: system-linux
 # RUN: llvm-mc -filetype=obj -triple x86_64-unknown-linux %s -o %t1.o
@@ -8,11 +9,10 @@
 # RUN: llvm-bolt --no-threads %t.exe --icf=safe -debug -debug-only=bolt-icf -o %t.bolt 2>&1 | FileCheck --check-prefix=SAFEICFCHECK %s
 
 # ICFCHECK:      ICF iteration 1
-# ICFCHECK-NEXT: folding _ZNK8Derived34funcEii into _ZNK8Derived24funcEii
 # ICFCHECK-NEXT: folding _ZN8Derived3D0Ev into _ZN8Derived2D0Ev
+# ICFCHECK-NEXT: folding _ZNK8Derived34funcEii into _ZNK8Derived24funcEii
 
-# SAFEICFCHECK:      skipping function _ZNK8Derived24funcEii
-# SAFEICFCHECK-NEXT: skipping function _ZNK8Derived34funcEii
+# SAFEICFCHECK: skipping function _ZNK8Derived34funcEii
 # SAFEICFCHECK-NEXT: ICF iteration 1
 # SAFEICFCHECK-NEXT: folding _ZN8Derived3D0Ev into _ZN8Derived2D0Ev
 # SAFEICFCHECK-NEXT: ===---------
@@ -88,51 +88,17 @@
 ##   return 5;
 ## }
 ## Manually modified to remove "extra" assembly.
-	.section	.text.hot.,"ax",@progbits
-	.globl	_Z10createTypei
-	.type	_Z10createTypei,@function
-_Z10createTypei:
-	callq	_Znwm@PLT
-	leaq	_ZTV8Derived2+16(%rip), %rcx
-	leaq	_ZTV8Derived3+16(%rip), %rdx
-	cmoveq	%rcx, %rdx
-	retq
-	.size	_Z10createTypei, .-_Z10createTypei
-
-	.globl	_Z10returnFivev
-	.type	_Z10returnFivev,@function
-_Z10returnFivev:
-	movl	$5, %eax
-	retq
-	.size	_Z10returnFivev, .-_Z10returnFivev
-
-	.globl	returnFourOrFiveFunc
-	.type	returnFourOrFiveFunc,@function
-returnFourOrFiveFunc:
-	xorl	%eax, %eax
-	cmpl	$1, %edi
-	sete	%al
-	xorl	$5, %eax
-	retq
-	.size	returnFourOrFiveFunc, .-returnFourOrFiveFunc
-
 	.globl	main
 	.type	main,@function
 main:
-	callq	returnFourOrFiveFunc@PLT
-	callq	_Z10returnFivev@PLT
-	callq	_Z10createTypei
-	callq	_Z10createTypei
-	leaq	_ZNK8Derived24funcEii(%rip), %rcx
-	callq	_ZNK8Derived24funcEii
 	leaq	_ZNK8Derived34funcEii(%rip), %rcx
 	callq	_ZNK8Derived34funcEii
 	.size	main, .-main
 
 	.section	.text.hot._ZNK8Derived24funcEii,"axG",@progbits,_ZNK8Derived24funcEii,comdat
 	.weak	_ZNK8Derived24funcEii
 	.type	_ZNK8Derived24funcEii,@function
-_ZNK8Derived24funcEii:                  #
+_ZNK8Derived24funcEii:
 	imull	%esi, %eax
 	retq
 	.size	_ZNK8Derived24funcEii, .-_ZNK8Derived24funcEii
@@ -141,7 +107,6 @@ _ZNK8Derived24funcEii:                  #
 	.weak	_ZN8Derived2D0Ev
 	.type	_ZN8Derived2D0Ev,@function
 _ZN8Derived2D0Ev:
-	movl	$16, %esi
 	jmp	_ZdlPvm@PLT
 	.size	_ZN8Derived2D0Ev, .-_ZN8Derived2D0Ev
 
@@ -164,7 +129,6 @@ _ZN4BaseD2Ev:
 	.weak	_ZN8Derived3D0Ev
 	.type	_ZN8Derived3D0Ev,@function
 _ZN8Derived3D0Ev:
-	movl	$16, %esi
 	jmp	_ZdlPvm@PLT
 	.size	_ZN8Derived3D0Ev, .-_ZN8Derived3D0Ev
 
@@ -179,37 +143,6 @@ _ZTV8Derived2:
 	.quad	_ZN8Derived2D0Ev
 	.size	_ZTV8Derived2, 40
 
-	.type	_ZTS8Derived2,@object
-	.section	.rodata._ZTS8Derived2,"aG",@progbits,_ZTS8Derived2,comdat
-	.weak	_ZTS8Derived2
-_ZTS8Derived2:
-	.asciz	"8Derived2"
-	.size	_ZTS8Derived2, 10
-
-	.type	_ZTS4Base,@object
-	.section	.rodata._ZTS4Base,"aG",@progbits,_ZTS4Base,comdat
-	.weak	_ZTS4Base
-_ZTS4Base:
-	.asciz	"4Base"
-	.size	_ZTS4Base, 6
-
-	.type	_ZTI4Base,@object
-	.section	.data.rel.ro._ZTI4Base,"awG",@progbits,_ZTI4Base,comdat
-	.weak	_ZTI4Base
-_ZTI4Base:
-	.quad	_ZTVN10__cxxabiv117__class_type_infoE+16
-	.quad	_ZTS4Base
-	.size	_ZTI4Base, 16
-
-	.type	_ZTI8Derived2,@object
-	.section	.data.rel.ro._ZTI8Derived2,"awG",@progbits,_ZTI8Derived2,comdat
-	.weak	_ZTI8Derived2
-_ZTI8Derived2:
-	.quad	_ZTVN10__cxxabiv120__si_class_type_infoE+16
-	.quad	_ZTS8Derived2
-	.quad	_ZTI4Base
-	.size	_ZTI8Derived2, 24
-
 	.type	_ZTV8Derived3,@object
 	.section	.data.rel.ro._ZTV8Derived3,"awG",@progbits,_ZTV8Derived3,comdat
 	.weak	_ZTV8Derived3
@@ -220,19 +153,3 @@ _ZTV8Derived3:
 	.quad	_ZN4BaseD2Ev
 	.quad	_ZN8Derived3D0Ev
 	.size	_ZTV8Derived3, 40
-
-	.type	_ZTS8Derived3,@object
-	.section	.rodata._ZTS8Derived3,"aG",@progbits,_ZTS8Derived3,comdat
-	.weak	_ZTS8Derived3
-_ZTS8Derived3:
-	.asciz	"8Derived3"
-	.size	_ZTS8Derived3, 10
-
-	.type	_ZTI8Derived3,@object
-	.section	.data.rel.ro._ZTI8Derived3,"awG",@progbits,_ZTI8Derived3,comdat
-	.weak	_ZTI8Derived3
-_ZTI8Derived3:
-	.quad	_ZTVN10__cxxabiv120__si_class_type_infoE+16
-	.quad	_ZTS8Derived3
-	.quad	_ZTI4Base
-	.size	_ZTI8Derived3, 24