[PATCH] D105911: [CallGraphSection] Introduce CGSectionFuncComdatCreator pass

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:1420
+
+if (CodeGenOpts.CallGraphSection) {
+  MPM.addPass(CGSectionFuncComdatCreatorPass());

https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements



Comment at: llvm/lib/Transforms/Utils/CGSectionFuncComdatCreator.cpp:64
+ F.hasAddressTaken(nullptr,
+   /* IgnoreCallbackUses */ true,
+   /* IgnoreAssumeLikeCalls */ true,

clang-format prefers `/*IgnoreCallbackUses=*/true`



Comment at: llvm/lib/Transforms/Utils/CGSectionFuncComdatCreator.cpp:91
+  ModuleCGSectionFuncComdatCreator ModuleCG;
+  if (ModuleCG.instrumentModule(M)) {
+return PreservedAnalyses::none();

delete braces



Comment at: llvm/test/Transforms/Util/create-function-comdats.ll:1
+; Tests that we create function comdats properly and only for those with no
+; comdats.

This should be within a call-graph-section specific directory.

Use `test/Instrumentation/InstrProfiling/comdat.ll` as a reference


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105911/new/

https://reviews.llvm.org/D105911

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D105911: [CallGraphSection] Introduce CGSectionFuncComdatCreator pass

[Necip Fazil Yildiran via Phabricator via cfe-commits]

necipfazil abandoned this revision.
necipfazil added a comment.

In D105911#2878345 , @morehouse wrote:

> Are comdats needed?  Can we get proper dead stripping with just 
> `SHF_LINK_ORDER`?

It looks like we can. I am abandoning this revision. I will shortly push the 
changes to related revisions for not using comdats.

> @MaskRay recently updated the documentation for associated metadata 
>  to imply that our 
> symbol doesn't need to share a comdat with its associated function when the 
> function doesn't have a comdat.
>
> Also, @MaskRay: Can adding comdats like this change the final code in the 
> fully-linked binary?




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105911/new/

https://reviews.llvm.org/D105911

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D105911: [CallGraphSection] Introduce CGSectionFuncComdatCreator pass

[Matt Morehouse via Phabricator via cfe-commits]

morehouse added a subscriber: MaskRay.
morehouse added a comment.

Are comdats needed?  Can we get proper dead stripping with just 
`SHF_LINK_ORDER`?

@MaskRay recently updated the documentation for associated metadata 
 to imply that our 
symbol doesn't need to share a comdat with its associated function when the 
function doesn't have a comdat.

Also, @MaskRay: Can adding comdats like this change the final code in the 
fully-linked binary?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105911/new/

https://reviews.llvm.org/D105911

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D105911: [CallGraphSection] Introduce CGSectionFuncComdatCreator pass

[Necip Fazil Yildiran via Phabricator via cfe-commits]

necipfazil created this revision.
necipfazil added reviewers: lattner, morehouse, kcc, llvm-commits.
Herald added subscribers: ormris, hiraditya, mgorny.
necipfazil requested review of this revision.
Herald added projects: clang, LLVM.
Herald added a subscriber: cfe-commits.

Create comdats for functions whose symbols will be referenced from the
call graph section. These comdats are used to create the call graph
sections, so that, the sections can get discarded by the linker if the
functions get removed.

Original RFC: https://lists.llvm.org/pipermail/llvm-dev/2021-June/151044.html
Updated RFC: https://lists.llvm.org/pipermail/llvm-dev/2021-July/151739.html


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D105911

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  llvm/include/llvm/Transforms/Utils/CGSectionFuncComdatCreator.h
  llvm/lib/Passes/PassBuilder.cpp
  llvm/lib/Passes/PassRegistry.def
  llvm/lib/Transforms/Utils/CGSectionFuncComdatCreator.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/test/Transforms/Util/create-function-comdats.ll

Index: llvm/test/Transforms/Util/create-function-comdats.ll
===
--- /dev/null
+++ llvm/test/Transforms/Util/create-function-comdats.ll
@@ -0,0 +1,21 @@
+; Tests that we create function comdats properly and only for those with no
+; comdats.
+
+; RUN: opt -passes='cg-section-func-comdat-creator' -S %s | FileCheck %s
+
+; CHECK: $f = comdat noduplicates
+; CHECK: $g = comdat any
+$g = comdat any
+
+; CHECK: define dso_local void @f() comdat {
+define dso_local void @f() {
+entry:
+  ret void
+}
+
+; CHECK: define dso_local void @g() comdat {
+define dso_local void @g() comdat {
+entry:
+  ret void
+}
+
Index: llvm/lib/Transforms/Utils/CMakeLists.txt
===
--- llvm/lib/Transforms/Utils/CMakeLists.txt
+++ llvm/lib/Transforms/Utils/CMakeLists.txt
@@ -7,6 +7,7 @@
   BreakCriticalEdges.cpp
   BuildLibCalls.cpp
   BypassSlowDivision.cpp
+  CGSectionFuncComdatCreator.cpp
   CallPromotionUtils.cpp
   CallGraphUpdater.cpp
   CanonicalizeAliases.cpp
Index: llvm/lib/Transforms/Utils/CGSectionFuncComdatCreator.cpp
===
--- /dev/null
+++ llvm/lib/Transforms/Utils/CGSectionFuncComdatCreator.cpp
@@ -0,0 +1,98 @@
+//===-- CGSectionFuncComdatCreator.cpp - CG section func comdat creator ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This pass creates comdats for functions whose symbols will be referenced
+// from the call graph section. These comdats are used to create the call graph
+// sections, so that, the sections can get discarded by the linker if the
+// functions get removed.
+//
+//===--===//
+
+#include "llvm/Transforms/Utils/CGSectionFuncComdatCreator.h"
+
+#include "llvm/ADT/Triple.h"
+#include "llvm/IR/Instructions.h"
+
+namespace llvm {
+
+namespace {
+class ModuleCGSectionFuncComdatCreator {
+public:
+  bool instrumentModule(Module &);
+
+private:
+  bool createFunctionComdat(Function , const Triple ) const;
+  bool hasIndirectCalls(const Function ) const;
+  bool isTargetToIndirectCalls(const Function ) const;
+};
+
+bool ModuleCGSectionFuncComdatCreator::createFunctionComdat(
+Function , const Triple ) const {
+  if (auto *Comdat = F.getComdat())
+return false;
+  assert(F.hasName());
+  Module *M = F.getParent();
+
+  // Make a new comdat for the function. Use the "no duplicates" selection kind
+  // if the object file format supports it. For COFF we restrict it to non-weak
+  // symbols.
+  Comdat *C = M->getOrInsertComdat(F.getName());
+  if (T.isOSBinFormatELF() || (T.isOSBinFormatCOFF() && !F.isWeakForLinker()))
+C->setSelectionKind(Comdat::NoDuplicates);
+  F.setComdat(C);
+  return true;
+}
+
+bool ModuleCGSectionFuncComdatCreator::hasIndirectCalls(
+const Function ) const {
+  for (const auto  : F)
+if (const CallInst *CI = dyn_cast())
+  if (CI->isIndirectCall())
+return true;
+  return false;
+}
+
+bool ModuleCGSectionFuncComdatCreator::isTargetToIndirectCalls(
+const Function ) const {
+  return !F.hasLocalLinkage() ||
+ F.hasAddressTaken(nullptr,
+   /* IgnoreCallbackUses */ true,
+   /* IgnoreAssumeLikeCalls */ true,
+   /* IgnoreLLVMUsed */ false);
+}
+
+bool ModuleCGSectionFuncComdatCreator::instrumentModule(Module ) {
+  Triple TargetTriple = Triple(M.getTargetTriple());
+
+  bool CreatedComdats = false;
+
+  for (Function  : M) {
+if (isTargetToIndirectCalls(F) || hasIndirectCalls(F)) {
+  if