[PATCH] D157750: Properly handle -fsplit-machine-functions for fatbinary compilation

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish added a subscriber: dhoekwater.
snehasish added inline comments.



Comment at: llvm/lib/CodeGen/TargetPassConfig.cpp:1278
 }
-addPass(createMachineFunctionSplitterPass());
+if (TM->getTargetTriple().isX86())
+  addPass(createMachineFunctionSplitterPass());

Can you coordinate with @dhoekwater ? He has some patches in flight for 
AArch64. 

I think D157157 is the one which modifies the same logic.



Comment at: llvm/test/CodeGen/X86/mfs-triple.ll:8
+
+define void @foo4(i1 zeroext %0, i1 zeroext %1) nounwind {
+  br i1 %0, label %3, label %7

Any reason why we can't use the bitcode already in 
test/CodeGen/machine-function-splitter.ll? (Going to be moved to 
test/Generic/machine-function-splitter.ll in D157563)

IMO we can just reuse the basic test and add these run and check lines.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157750

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

[PATCH] D154856: [MemProf] Use new option/pass for profile feedback and matching

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish accepted this revision.
snehasish added a comment.
This revision is now accepted and ready to land.

lgtm, thanks for testing the various combinations of profiles passed to each 
option.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154856

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

[PATCH] D154856: [MemProf] Use new option/pass for profile feedback and matching

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish added inline comments.



Comment at: llvm/lib/Transforms/Instrumentation/MemProfiler.cpp:912
+const TargetLibraryInfo  = FAM.getResult(F);
+readMemprof(M, F, MemProfReader.get(), TLI);
+  }

I think we can we split this patch into two to make the changes clearer. 
Consider the following -- 

First move the readMemprof and its callees to MemProfiler.cpp and call it from 
the original code. Also in this step consider using Error as the return type of 
readMemprof, right now the bool returned is ignored. Using Error doesn't need 
to be fatal, we can handle it and emit a warning but it would enforce the check 
I think.

Second, add the pass to MemProfiler and all the related options/plumbing.

What do you think? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154856

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

[PATCH] D151593: [MemProf] Clean up MemProf instrumentation pass invocation

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish accepted this revision.
snehasish added a comment.

lgtm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151593

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

[PATCH] D149215: [MemProf] Control availability of hot/cold operator new from LTO link

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish accepted this revision.
snehasish added a comment.
This revision is now accepted and ready to land.

lgtm




Comment at: llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp:3163
 
+  // TODO: If/when other types of memprof cloning are enabled beyond just for
+  // hot and cold, we will need to change this to individually control the

Thanks for adding this comment to clarify


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149215

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

[PATCH] D149600: [MemProf] Recognize hot/cold operator new as replaceable allocations

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish accepted this revision.
snehasish added a comment.
This revision is now accepted and ready to land.

lgtm




Comment at: clang/test/CodeGenCXX/new_hot_cold.cpp:16
+
+enum class __hot_cold_t : uint8_t;
+namespace malloc_namespace {

Can we skip the typedef and just say `enum class __hot_cold_t : unsigned char;`?

Also it might be useful to link to the tcmalloc documentation for hot_cold_t to 
document the expected underlying type?
https://github.com/google/tcmalloc/blob/220043886d4e2efff7a5702d5172cb8065253664/tcmalloc/malloc_extension.h#L53




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149600

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

[PATCH] D145190: [memprof] Record BuildIDs in the raw profile.

[Snehasish Kumar via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa1bbf5ac3cbd: [memprof] Record BuildIDs in the raw profile. 
(authored by snehasish).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145190

Files:
  clang/test/CodeGen/Inputs/memprof.exe
  clang/test/CodeGen/Inputs/memprof.memprofraw
  compiler-rt/include/profile/MemProfData.inc
  compiler-rt/lib/memprof/memprof_allocator.cpp
  compiler-rt/lib/memprof/memprof_rawprofile.cpp
  compiler-rt/lib/memprof/memprof_rawprofile.h
  compiler-rt/lib/memprof/tests/rawprofile.cpp
  llvm/include/llvm/ProfileData/MemProfData.inc
  llvm/lib/ProfileData/RawMemProfReader.cpp
  llvm/test/Transforms/PGOProfile/Inputs/memprof.exe
  llvm/test/Transforms/PGOProfile/Inputs/memprof.memprofraw
  llvm/test/Transforms/PGOProfile/Inputs/memprof_pgo.profraw
  llvm/test/tools/llvm-profdata/Inputs/basic.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/basic.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/buildid.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/buildid.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/inline.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/inline.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/multi.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/multi.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/pic.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/pic.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/update_memprof_inputs.sh
  llvm/test/tools/llvm-profdata/memprof-basic.test
  llvm/test/tools/llvm-profdata/memprof-buildid.test
  llvm/test/tools/llvm-profdata/memprof-inline.test
  llvm/test/tools/llvm-profdata/memprof-multi.test

Index: llvm/test/tools/llvm-profdata/memprof-multi.test
===
--- llvm/test/tools/llvm-profdata/memprof-multi.test
+++ llvm/test/tools/llvm-profdata/memprof-multi.test
@@ -7,7 +7,7 @@
 
 CHECK:  MemprofProfile:
 CHECK-NEXT:  Summary:
-CHECK-NEXT:Version: 2
+CHECK-NEXT:Version: 3
 CHECK-NEXT:NumSegments: {{[0-9]+}}
 CHECK-NEXT:NumMibInfo: 2
 CHECK-NEXT:NumAllocFunctions: 1
Index: llvm/test/tools/llvm-profdata/memprof-inline.test
===
--- llvm/test/tools/llvm-profdata/memprof-inline.test
+++ llvm/test/tools/llvm-profdata/memprof-inline.test
@@ -5,17 +5,17 @@
 
 CHECK:  MemprofProfile:
 CHECK-NEXT:  Summary:
-CHECK-NEXT:Version: 2
+CHECK-NEXT:Version: 3
 CHECK-NEXT:NumSegments: {{[0-9]+}}
 CHECK-NEXT:NumMibInfo: 2
 CHECK-NEXT:NumAllocFunctions: 2
 CHECK-NEXT:NumStackOffsets: 1
 CHECK-NEXT:  Segments:
 CHECK-NEXT:  -
-CHECK-NEXT:BuildId: 
-CHECK-NEXT:Start: 0x{{[0-9]+}}
-CHECK-NEXT:End: 0x{{[0-9]+}}
-CHECK-NEXT:Offset: 0x{{[0-9]+}}
+CHECK-NEXT:BuildId: {{[[:xdigit:]]+}}
+CHECK-NEXT:Start: 0x{{[[:xdigit:]]+}}
+CHECK-NEXT:End: 0x{{[[:xdigit:]]+}}
+CHECK-NEXT:Offset: 0x{{[[:xdigit:]]+}}
 CHECK-NEXT:  -
 
 CHECK:  Records:
Index: llvm/test/tools/llvm-profdata/memprof-buildid.test
===
--- /dev/null
+++ llvm/test/tools/llvm-profdata/memprof-buildid.test
@@ -0,0 +1,12 @@
+REQUIRES: x86_64-linux
+
+To update the inputs used below run Inputs/update_memprof_inputs.sh /path/to/updated/clang
+RUN: llvm-readelf --notes %p/Inputs/buildid.memprofexe > %t1.txt
+RUN: llvm-profdata show --memory %p/Inputs/buildid.memprofraw --profiled-binary %p/Inputs/buildid.memprofexe -o -  > %t2.txt
+RUN: cat %t1.txt %t2.txt | FileCheck %s
+
+COM: First extract the id from the llvm-readelf output.
+CHECK: Build ID: [[ID:[[:xdigit:]]+]]
+
+COM: Then match it with the profdata output.
+CHECK: BuildId: {{.*}}[[ID]]
Index: llvm/test/tools/llvm-profdata/memprof-basic.test
===
--- llvm/test/tools/llvm-profdata/memprof-basic.test
+++ llvm/test/tools/llvm-profdata/memprof-basic.test
@@ -8,17 +8,17 @@
 
 CHECK:  MemprofProfile:
 CHECK-NEXT:   Summary:
-CHECK-NEXT: Version: 2
+CHECK-NEXT: Version: 3
 CHECK-NEXT: NumSegments: {{[0-9]+}}
 CHECK-NEXT: NumMibInfo: 2
 CHECK-NEXT: NumAllocFunctions: 1
 CHECK-NEXT: NumStackOffsets: 2
 CHECK-NEXT:   Segments:
 CHECK-NEXT:   -
-CHECK-NEXT: BuildId: 
-CHECK-NEXT: Start: 0x{{[0-9]+}}
-CHECK-NEXT: End: 0x{{[0-9]+}}
-CHECK-NEXT: Offset: 0x{{[0-9]+}}
+CHECK-NEXT: BuildId: {{[[:xdigit:]]+}}
+CHECK-NEXT: Start: 0x{{[[:xdigit:]]+}}
+CHECK-NEXT: End: 0x{{[[:xdigit:]]+}}
+CHECK-NEXT: Offset: 0x{{[[:xdigit:]]+}}
 CHECK-NEXT:   -
 
 CHECK:   Records:
Index: llvm/test/tools/llvm-profdata/Inputs/update_memprof_inputs.sh
===
--- llvm/test/tools/llvm-profdata/Inputs/update_memprof_inputs.sh
+++ llvm/test/tools/llvm-profdata/Inputs/update_memprof_inputs.sh
@@ -72,6 

[PATCH] D145190: [memprof] Record BuildIDs in the raw profile.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish updated this revision to Diff 504887.
snehasish added a comment.

Update the unittest in compiler-rt which was caught by the build bot failures.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145190

Files:
  clang/test/CodeGen/Inputs/memprof.exe
  clang/test/CodeGen/Inputs/memprof.memprofraw
  compiler-rt/include/profile/MemProfData.inc
  compiler-rt/lib/memprof/memprof_allocator.cpp
  compiler-rt/lib/memprof/memprof_rawprofile.cpp
  compiler-rt/lib/memprof/memprof_rawprofile.h
  compiler-rt/lib/memprof/tests/rawprofile.cpp
  llvm/include/llvm/ProfileData/MemProfData.inc
  llvm/lib/ProfileData/RawMemProfReader.cpp
  llvm/test/Transforms/PGOProfile/Inputs/memprof.exe
  llvm/test/Transforms/PGOProfile/Inputs/memprof.memprofraw
  llvm/test/Transforms/PGOProfile/Inputs/memprof_pgo.profraw
  llvm/test/tools/llvm-profdata/Inputs/basic.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/basic.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/buildid.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/buildid.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/inline.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/inline.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/multi.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/multi.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/pic.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/pic.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/update_memprof_inputs.sh
  llvm/test/tools/llvm-profdata/memprof-basic.test
  llvm/test/tools/llvm-profdata/memprof-buildid.test
  llvm/test/tools/llvm-profdata/memprof-inline.test
  llvm/test/tools/llvm-profdata/memprof-multi.test

Index: llvm/test/tools/llvm-profdata/memprof-multi.test
===
--- llvm/test/tools/llvm-profdata/memprof-multi.test
+++ llvm/test/tools/llvm-profdata/memprof-multi.test
@@ -7,7 +7,7 @@
 
 CHECK:  MemprofProfile:
 CHECK-NEXT:  Summary:
-CHECK-NEXT:Version: 2
+CHECK-NEXT:Version: 3
 CHECK-NEXT:NumSegments: {{[0-9]+}}
 CHECK-NEXT:NumMibInfo: 2
 CHECK-NEXT:NumAllocFunctions: 1
Index: llvm/test/tools/llvm-profdata/memprof-inline.test
===
--- llvm/test/tools/llvm-profdata/memprof-inline.test
+++ llvm/test/tools/llvm-profdata/memprof-inline.test
@@ -5,17 +5,17 @@
 
 CHECK:  MemprofProfile:
 CHECK-NEXT:  Summary:
-CHECK-NEXT:Version: 2
+CHECK-NEXT:Version: 3
 CHECK-NEXT:NumSegments: {{[0-9]+}}
 CHECK-NEXT:NumMibInfo: 2
 CHECK-NEXT:NumAllocFunctions: 2
 CHECK-NEXT:NumStackOffsets: 1
 CHECK-NEXT:  Segments:
 CHECK-NEXT:  -
-CHECK-NEXT:BuildId: 
-CHECK-NEXT:Start: 0x{{[0-9]+}}
-CHECK-NEXT:End: 0x{{[0-9]+}}
-CHECK-NEXT:Offset: 0x{{[0-9]+}}
+CHECK-NEXT:BuildId: {{[[:xdigit:]]+}}
+CHECK-NEXT:Start: 0x{{[[:xdigit:]]+}}
+CHECK-NEXT:End: 0x{{[[:xdigit:]]+}}
+CHECK-NEXT:Offset: 0x{{[[:xdigit:]]+}}
 CHECK-NEXT:  -
 
 CHECK:  Records:
Index: llvm/test/tools/llvm-profdata/memprof-buildid.test
===
--- /dev/null
+++ llvm/test/tools/llvm-profdata/memprof-buildid.test
@@ -0,0 +1,12 @@
+REQUIRES: x86_64-linux
+
+To update the inputs used below run Inputs/update_memprof_inputs.sh /path/to/updated/clang
+RUN: llvm-readelf --notes %p/Inputs/buildid.memprofexe > %t1.txt
+RUN: llvm-profdata show --memory %p/Inputs/buildid.memprofraw --profiled-binary %p/Inputs/buildid.memprofexe -o -  > %t2.txt
+RUN: cat %t1.txt %t2.txt | FileCheck %s
+
+COM: First extract the id from the llvm-readelf output.
+CHECK: Build ID: [[ID:[[:xdigit:]]+]]
+
+COM: Then match it with the profdata output.
+CHECK: BuildId: {{.*}}[[ID]]
Index: llvm/test/tools/llvm-profdata/memprof-basic.test
===
--- llvm/test/tools/llvm-profdata/memprof-basic.test
+++ llvm/test/tools/llvm-profdata/memprof-basic.test
@@ -8,17 +8,17 @@
 
 CHECK:  MemprofProfile:
 CHECK-NEXT:   Summary:
-CHECK-NEXT: Version: 2
+CHECK-NEXT: Version: 3
 CHECK-NEXT: NumSegments: {{[0-9]+}}
 CHECK-NEXT: NumMibInfo: 2
 CHECK-NEXT: NumAllocFunctions: 1
 CHECK-NEXT: NumStackOffsets: 2
 CHECK-NEXT:   Segments:
 CHECK-NEXT:   -
-CHECK-NEXT: BuildId: 
-CHECK-NEXT: Start: 0x{{[0-9]+}}
-CHECK-NEXT: End: 0x{{[0-9]+}}
-CHECK-NEXT: Offset: 0x{{[0-9]+}}
+CHECK-NEXT: BuildId: {{[[:xdigit:]]+}}
+CHECK-NEXT: Start: 0x{{[[:xdigit:]]+}}
+CHECK-NEXT: End: 0x{{[[:xdigit:]]+}}
+CHECK-NEXT: Offset: 0x{{[[:xdigit:]]+}}
 CHECK-NEXT:   -
 
 CHECK:   Records:
Index: llvm/test/tools/llvm-profdata/Inputs/update_memprof_inputs.sh
===
--- llvm/test/tools/llvm-profdata/Inputs/update_memprof_inputs.sh
+++ llvm/test/tools/llvm-profdata/Inputs/update_memprof_inputs.sh
@@ -72,6 +72,7 @@
 

[PATCH] D145190: [memprof] Record BuildIDs in the raw profile.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish reopened this revision.
snehasish added a comment.
This revision is now accepted and ready to land.

Reopening since I missed the updates to the memprof unit tests in compiler-rt. 
This was caught by the buildbots. As part of the changes I modified the 
`SerializeToRawProfile` API slightly to make it easier to test. Also 
regenerated the raw profiles and binaries again since the runtime changed.

@tejohnson PTAL, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145190

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

[PATCH] D145190: [memprof] Record BuildIDs in the raw profile.

[Snehasish Kumar via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG287177a47a39: [memprof] Record BuildIDs in the raw profile. 
(authored by snehasish).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145190

Files:
  clang/test/CodeGen/Inputs/memprof.exe
  clang/test/CodeGen/Inputs/memprof.memprofraw
  compiler-rt/include/profile/MemProfData.inc
  compiler-rt/lib/memprof/memprof_allocator.cpp
  compiler-rt/lib/memprof/memprof_rawprofile.cpp
  compiler-rt/lib/memprof/memprof_rawprofile.h
  llvm/include/llvm/ProfileData/MemProfData.inc
  llvm/lib/ProfileData/RawMemProfReader.cpp
  llvm/test/Transforms/PGOProfile/Inputs/memprof.exe
  llvm/test/Transforms/PGOProfile/Inputs/memprof.memprofraw
  llvm/test/Transforms/PGOProfile/Inputs/memprof_pgo.profraw
  llvm/test/tools/llvm-profdata/Inputs/basic.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/basic.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/buildid.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/buildid.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/inline.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/inline.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/multi.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/multi.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/pic.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/pic.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/update_memprof_inputs.sh
  llvm/test/tools/llvm-profdata/memprof-basic.test
  llvm/test/tools/llvm-profdata/memprof-buildid.test
  llvm/test/tools/llvm-profdata/memprof-inline.test
  llvm/test/tools/llvm-profdata/memprof-multi.test

Index: llvm/test/tools/llvm-profdata/memprof-multi.test
===
--- llvm/test/tools/llvm-profdata/memprof-multi.test
+++ llvm/test/tools/llvm-profdata/memprof-multi.test
@@ -7,7 +7,7 @@
 
 CHECK:  MemprofProfile:
 CHECK-NEXT:  Summary:
-CHECK-NEXT:Version: 2
+CHECK-NEXT:Version: 3
 CHECK-NEXT:NumSegments: {{[0-9]+}}
 CHECK-NEXT:NumMibInfo: 2
 CHECK-NEXT:NumAllocFunctions: 1
Index: llvm/test/tools/llvm-profdata/memprof-inline.test
===
--- llvm/test/tools/llvm-profdata/memprof-inline.test
+++ llvm/test/tools/llvm-profdata/memprof-inline.test
@@ -5,17 +5,17 @@
 
 CHECK:  MemprofProfile:
 CHECK-NEXT:  Summary:
-CHECK-NEXT:Version: 2
+CHECK-NEXT:Version: 3
 CHECK-NEXT:NumSegments: {{[0-9]+}}
 CHECK-NEXT:NumMibInfo: 2
 CHECK-NEXT:NumAllocFunctions: 2
 CHECK-NEXT:NumStackOffsets: 1
 CHECK-NEXT:  Segments:
 CHECK-NEXT:  -
-CHECK-NEXT:BuildId: 
-CHECK-NEXT:Start: 0x{{[0-9]+}}
-CHECK-NEXT:End: 0x{{[0-9]+}}
-CHECK-NEXT:Offset: 0x{{[0-9]+}}
+CHECK-NEXT:BuildId: {{[[:xdigit:]]+}}
+CHECK-NEXT:Start: 0x{{[[:xdigit:]]+}}
+CHECK-NEXT:End: 0x{{[[:xdigit:]]+}}
+CHECK-NEXT:Offset: 0x{{[[:xdigit:]]+}}
 CHECK-NEXT:  -
 
 CHECK:  Records:
Index: llvm/test/tools/llvm-profdata/memprof-buildid.test
===
--- /dev/null
+++ llvm/test/tools/llvm-profdata/memprof-buildid.test
@@ -0,0 +1,12 @@
+REQUIRES: x86_64-linux
+
+To update the inputs used below run Inputs/update_memprof_inputs.sh /path/to/updated/clang
+RUN: llvm-readelf --notes %p/Inputs/buildid.memprofexe > %t1.txt
+RUN: llvm-profdata show --memory %p/Inputs/buildid.memprofraw --profiled-binary %p/Inputs/buildid.memprofexe -o -  > %t2.txt
+RUN: cat %t1.txt %t2.txt | FileCheck %s
+
+COM: First extract the id from the llvm-readelf output.
+CHECK: Build ID: [[ID:[[:xdigit:]]+]]
+
+COM: Then match it with the profdata output.
+CHECK: BuildId: {{.*}}[[ID]]
Index: llvm/test/tools/llvm-profdata/memprof-basic.test
===
--- llvm/test/tools/llvm-profdata/memprof-basic.test
+++ llvm/test/tools/llvm-profdata/memprof-basic.test
@@ -8,17 +8,17 @@
 
 CHECK:  MemprofProfile:
 CHECK-NEXT:   Summary:
-CHECK-NEXT: Version: 2
+CHECK-NEXT: Version: 3
 CHECK-NEXT: NumSegments: {{[0-9]+}}
 CHECK-NEXT: NumMibInfo: 2
 CHECK-NEXT: NumAllocFunctions: 1
 CHECK-NEXT: NumStackOffsets: 2
 CHECK-NEXT:   Segments:
 CHECK-NEXT:   -
-CHECK-NEXT: BuildId: 
-CHECK-NEXT: Start: 0x{{[0-9]+}}
-CHECK-NEXT: End: 0x{{[0-9]+}}
-CHECK-NEXT: Offset: 0x{{[0-9]+}}
+CHECK-NEXT: BuildId: {{[[:xdigit:]]+}}
+CHECK-NEXT: Start: 0x{{[[:xdigit:]]+}}
+CHECK-NEXT: End: 0x{{[[:xdigit:]]+}}
+CHECK-NEXT: Offset: 0x{{[[:xdigit:]]+}}
 CHECK-NEXT:   -
 
 CHECK:   Records:
Index: llvm/test/tools/llvm-profdata/Inputs/update_memprof_inputs.sh
===
--- llvm/test/tools/llvm-profdata/Inputs/update_memprof_inputs.sh
+++ llvm/test/tools/llvm-profdata/Inputs/update_memprof_inputs.sh
@@ -72,6 +72,7 @@
 INPUTS["inline"]="INLINE"
 

[PATCH] D145190: [memprof] Record BuildIDs in the raw profile.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish updated this revision to Diff 503945.
snehasish added a comment.

Update test regex.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145190

Files:
  clang/test/CodeGen/Inputs/memprof.exe
  clang/test/CodeGen/Inputs/memprof.memprofraw
  compiler-rt/include/profile/MemProfData.inc
  compiler-rt/lib/memprof/memprof_allocator.cpp
  compiler-rt/lib/memprof/memprof_rawprofile.cpp
  compiler-rt/lib/memprof/memprof_rawprofile.h
  llvm/include/llvm/ProfileData/MemProfData.inc
  llvm/lib/ProfileData/RawMemProfReader.cpp
  llvm/test/Transforms/PGOProfile/Inputs/memprof.exe
  llvm/test/Transforms/PGOProfile/Inputs/memprof.memprofraw
  llvm/test/Transforms/PGOProfile/Inputs/memprof_pgo.profraw
  llvm/test/tools/llvm-profdata/Inputs/basic.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/basic.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/buildid.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/buildid.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/inline.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/inline.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/multi.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/multi.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/pic.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/pic.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/update_memprof_inputs.sh
  llvm/test/tools/llvm-profdata/memprof-basic.test
  llvm/test/tools/llvm-profdata/memprof-buildid.test
  llvm/test/tools/llvm-profdata/memprof-inline.test
  llvm/test/tools/llvm-profdata/memprof-multi.test

Index: llvm/test/tools/llvm-profdata/memprof-multi.test
===
--- llvm/test/tools/llvm-profdata/memprof-multi.test
+++ llvm/test/tools/llvm-profdata/memprof-multi.test
@@ -7,7 +7,7 @@
 
 CHECK:  MemprofProfile:
 CHECK-NEXT:  Summary:
-CHECK-NEXT:Version: 2
+CHECK-NEXT:Version: 3
 CHECK-NEXT:NumSegments: {{[0-9]+}}
 CHECK-NEXT:NumMibInfo: 2
 CHECK-NEXT:NumAllocFunctions: 1
Index: llvm/test/tools/llvm-profdata/memprof-inline.test
===
--- llvm/test/tools/llvm-profdata/memprof-inline.test
+++ llvm/test/tools/llvm-profdata/memprof-inline.test
@@ -5,17 +5,17 @@
 
 CHECK:  MemprofProfile:
 CHECK-NEXT:  Summary:
-CHECK-NEXT:Version: 2
+CHECK-NEXT:Version: 3
 CHECK-NEXT:NumSegments: {{[0-9]+}}
 CHECK-NEXT:NumMibInfo: 2
 CHECK-NEXT:NumAllocFunctions: 2
 CHECK-NEXT:NumStackOffsets: 1
 CHECK-NEXT:  Segments:
 CHECK-NEXT:  -
-CHECK-NEXT:BuildId: 
-CHECK-NEXT:Start: 0x{{[0-9]+}}
-CHECK-NEXT:End: 0x{{[0-9]+}}
-CHECK-NEXT:Offset: 0x{{[0-9]+}}
+CHECK-NEXT:BuildId: {{[[:xdigit:]]+}}
+CHECK-NEXT:Start: 0x{{[[:xdigit:]]+}}
+CHECK-NEXT:End: 0x{{[[:xdigit:]]+}}
+CHECK-NEXT:Offset: 0x{{[[:xdigit:]]+}}
 CHECK-NEXT:  -
 
 CHECK:  Records:
Index: llvm/test/tools/llvm-profdata/memprof-buildid.test
===
--- /dev/null
+++ llvm/test/tools/llvm-profdata/memprof-buildid.test
@@ -0,0 +1,12 @@
+REQUIRES: x86_64-linux
+
+To update the inputs used below run Inputs/update_memprof_inputs.sh /path/to/updated/clang
+RUN: llvm-readelf --notes %p/Inputs/buildid.memprofexe > %t1.txt
+RUN: llvm-profdata show --memory %p/Inputs/buildid.memprofraw --profiled-binary %p/Inputs/buildid.memprofexe -o -  > %t2.txt
+RUN: cat %t1.txt %t2.txt | FileCheck %s
+
+COM: First extract the id from the llvm-readelf output.
+CHECK: Build ID: [[ID:[[:xdigit:]]+]]
+
+COM: Then match it with the profdata output.
+CHECK: BuildId: {{.*}}[[ID]]
Index: llvm/test/tools/llvm-profdata/memprof-basic.test
===
--- llvm/test/tools/llvm-profdata/memprof-basic.test
+++ llvm/test/tools/llvm-profdata/memprof-basic.test
@@ -8,17 +8,17 @@
 
 CHECK:  MemprofProfile:
 CHECK-NEXT:   Summary:
-CHECK-NEXT: Version: 2
+CHECK-NEXT: Version: 3
 CHECK-NEXT: NumSegments: {{[0-9]+}}
 CHECK-NEXT: NumMibInfo: 2
 CHECK-NEXT: NumAllocFunctions: 1
 CHECK-NEXT: NumStackOffsets: 2
 CHECK-NEXT:   Segments:
 CHECK-NEXT:   -
-CHECK-NEXT: BuildId: 
-CHECK-NEXT: Start: 0x{{[0-9]+}}
-CHECK-NEXT: End: 0x{{[0-9]+}}
-CHECK-NEXT: Offset: 0x{{[0-9]+}}
+CHECK-NEXT: BuildId: {{[[:xdigit:]]+}}
+CHECK-NEXT: Start: 0x{{[[:xdigit:]]+}}
+CHECK-NEXT: End: 0x{{[[:xdigit:]]+}}
+CHECK-NEXT: Offset: 0x{{[[:xdigit:]]+}}
 CHECK-NEXT:   -
 
 CHECK:   Records:
Index: llvm/test/tools/llvm-profdata/Inputs/update_memprof_inputs.sh
===
--- llvm/test/tools/llvm-profdata/Inputs/update_memprof_inputs.sh
+++ llvm/test/tools/llvm-profdata/Inputs/update_memprof_inputs.sh
@@ -72,6 +72,7 @@
 INPUTS["inline"]="INLINE"
 INPUTS["multi"]="MULTI"
 INPUTS["pic"]="BASIC;-pie"

[PATCH] D145190: [memprof] Record BuildIDs in the raw profile.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish added a comment.

In D145190#4181789 , @snehasish wrote:

> In D145190#4181608 , @tejohnson 
> wrote:
>
>> lgtm - but does this depend on D145644  
>> and require some additional test changes?
>
> Yes, I will rebase this patch once that is submitted. I need to look at the 
> LLVM.Transforms/PGOProfile::memprof.ll test failures.

@tejohnson I've rebased the patch and updated the raw profiles for all the 
tests with the new version. Also added a new test for llvm-profdata to check 
the buildid we serialize in the raw profile. No changes to the code from prior 
diffs. PTAL, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145190

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

[PATCH] D145190: [memprof] Record BuildIDs in the raw profile.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish updated this revision to Diff 503938.
snehasish added a comment.
Herald added subscribers: cfe-commits, wenlei.
Herald added a project: clang.

Update raw profiles, add another test for buildids only.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145190

Files:
  clang/test/CodeGen/Inputs/memprof.exe
  clang/test/CodeGen/Inputs/memprof.memprofraw
  compiler-rt/include/profile/MemProfData.inc
  compiler-rt/lib/memprof/memprof_allocator.cpp
  compiler-rt/lib/memprof/memprof_rawprofile.cpp
  compiler-rt/lib/memprof/memprof_rawprofile.h
  llvm/include/llvm/ProfileData/MemProfData.inc
  llvm/lib/ProfileData/RawMemProfReader.cpp
  llvm/test/Transforms/PGOProfile/Inputs/memprof.exe
  llvm/test/Transforms/PGOProfile/Inputs/memprof.memprofraw
  llvm/test/Transforms/PGOProfile/Inputs/memprof_pgo.profraw
  llvm/test/tools/llvm-profdata/Inputs/basic.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/basic.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/buildid.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/buildid.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/inline.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/inline.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/multi.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/multi.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/pic.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/pic.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/update_memprof_inputs.sh
  llvm/test/tools/llvm-profdata/memprof-basic.test
  llvm/test/tools/llvm-profdata/memprof-buildid.test
  llvm/test/tools/llvm-profdata/memprof-inline.test
  llvm/test/tools/llvm-profdata/memprof-multi.test

Index: llvm/test/tools/llvm-profdata/memprof-multi.test
===
--- llvm/test/tools/llvm-profdata/memprof-multi.test
+++ llvm/test/tools/llvm-profdata/memprof-multi.test
@@ -7,7 +7,7 @@
 
 CHECK:  MemprofProfile:
 CHECK-NEXT:  Summary:
-CHECK-NEXT:Version: 2
+CHECK-NEXT:Version: 3
 CHECK-NEXT:NumSegments: {{[0-9]+}}
 CHECK-NEXT:NumMibInfo: 2
 CHECK-NEXT:NumAllocFunctions: 1
Index: llvm/test/tools/llvm-profdata/memprof-inline.test
===
--- llvm/test/tools/llvm-profdata/memprof-inline.test
+++ llvm/test/tools/llvm-profdata/memprof-inline.test
@@ -5,17 +5,17 @@
 
 CHECK:  MemprofProfile:
 CHECK-NEXT:  Summary:
-CHECK-NEXT:Version: 2
+CHECK-NEXT:Version: 3
 CHECK-NEXT:NumSegments: {{[0-9]+}}
 CHECK-NEXT:NumMibInfo: 2
 CHECK-NEXT:NumAllocFunctions: 2
 CHECK-NEXT:NumStackOffsets: 1
 CHECK-NEXT:  Segments:
 CHECK-NEXT:  -
-CHECK-NEXT:BuildId: 
-CHECK-NEXT:Start: 0x{{[0-9]+}}
-CHECK-NEXT:End: 0x{{[0-9]+}}
-CHECK-NEXT:Offset: 0x{{[0-9]+}}
+CHECK-NEXT:BuildId: {{[[:xdigit:]]}}
+CHECK-NEXT:Start: 0x{{[[:xdigit:]]}}
+CHECK-NEXT:End: 0x{{[[:xdigit:]]}}
+CHECK-NEXT:Offset: 0x{{[[:xdigit:]]}}
 CHECK-NEXT:  -
 
 CHECK:  Records:
Index: llvm/test/tools/llvm-profdata/memprof-buildid.test
===
--- /dev/null
+++ llvm/test/tools/llvm-profdata/memprof-buildid.test
@@ -0,0 +1,12 @@
+REQUIRES: x86_64-linux
+
+To update the inputs used below run Inputs/update_memprof_inputs.sh /path/to/updated/clang
+RUN: llvm-readelf --notes %p/Inputs/buildid.memprofexe > %t1.txt
+RUN: llvm-profdata show --memory %p/Inputs/buildid.memprofraw --profiled-binary %p/Inputs/buildid.memprofexe -o -  > %t2.txt
+RUN: cat %t1.txt %t2.txt | FileCheck %s
+
+COM: First extract the id from the llvm-readelf output.
+CHECK: Build ID: [[ID:[[:xdigit:]]+]]
+
+COM: Then match it with the profdata output.
+CHECK: BuildId: {{.*}}[[ID]]
Index: llvm/test/tools/llvm-profdata/memprof-basic.test
===
--- llvm/test/tools/llvm-profdata/memprof-basic.test
+++ llvm/test/tools/llvm-profdata/memprof-basic.test
@@ -8,17 +8,17 @@
 
 CHECK:  MemprofProfile:
 CHECK-NEXT:   Summary:
-CHECK-NEXT: Version: 2
+CHECK-NEXT: Version: 3
 CHECK-NEXT: NumSegments: {{[0-9]+}}
 CHECK-NEXT: NumMibInfo: 2
 CHECK-NEXT: NumAllocFunctions: 1
 CHECK-NEXT: NumStackOffsets: 2
 CHECK-NEXT:   Segments:
 CHECK-NEXT:   -
-CHECK-NEXT: BuildId: 
-CHECK-NEXT: Start: 0x{{[0-9]+}}
-CHECK-NEXT: End: 0x{{[0-9]+}}
-CHECK-NEXT: Offset: 0x{{[0-9]+}}
+CHECK-NEXT: BuildId: {{[[:xdigit:]]}}
+CHECK-NEXT: Start: 0x{{[[:xdigit:]]}}
+CHECK-NEXT: End: 0x{{[[:xdigit:]]}}
+CHECK-NEXT: Offset: 0x{{[[:xdigit:]]}}
 CHECK-NEXT:   -
 
 CHECK:   Records:
Index: llvm/test/tools/llvm-profdata/Inputs/update_memprof_inputs.sh
===
--- llvm/test/tools/llvm-profdata/Inputs/update_memprof_inputs.sh
+++ llvm/test/tools/llvm-profdata/Inputs/update_memprof_inputs.sh
@@ -72,6 +72,7 @@
 

[PATCH] D145644: [memprof] Add scripts to automate testdata regeneration.

[Snehasish Kumar via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGe99b5ad38381: [memprof] Add scripts to automate testdata 
regeneration. (authored by snehasish).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145644

Files:
  clang/test/CodeGen/Inputs/memprof.exe
  clang/test/CodeGen/Inputs/memprof.memprofraw
  clang/test/CodeGen/Inputs/update_memprof_inputs.sh
  clang/test/CodeGen/memprof.cpp
  llvm/test/Transforms/PGOProfile/Inputs/memprof.exe
  llvm/test/Transforms/PGOProfile/Inputs/memprof.memprofraw
  llvm/test/Transforms/PGOProfile/Inputs/memprof_pgo.profraw
  llvm/test/Transforms/PGOProfile/Inputs/update_memprof_inputs.sh
  llvm/test/Transforms/PGOProfile/memprof.ll
  llvm/test/tools/llvm-profdata/Inputs/basic.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/basic.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/inline.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/inline.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/memprof-inline.exe
  llvm/test/tools/llvm-profdata/Inputs/multi.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/multi.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/pic.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/pic.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/update_memprof_inputs.sh
  llvm/test/tools/llvm-profdata/memprof-basic.test
  llvm/test/tools/llvm-profdata/memprof-inline.test
  llvm/test/tools/llvm-profdata/memprof-merge.test
  llvm/test/tools/llvm-profdata/memprof-multi.test
  llvm/test/tools/llvm-profdata/memprof-pic.test

Index: llvm/test/tools/llvm-profdata/memprof-pic.test
===
--- llvm/test/tools/llvm-profdata/memprof-pic.test
+++ llvm/test/tools/llvm-profdata/memprof-pic.test
@@ -1,40 +1,12 @@
 REQUIRES: x86_64-linux
 
-This test ensures that llvm-profdata fails with a descriptive error message
-when invoked on a memprof profiled binary which was built with position
-independent code.
-
-The input raw profile test has been generated from the following source code:
-
-```
-#include 
-#include 
-int main(int argc, char **argv) {
-  char *x = (char *)malloc(10);
-  memset(x, 0, 10);
-  free(x);
-  x = (char *)malloc(10);
-  memset(x, 0, 10);
-  free(x);
-  return 0;
-}
-```
-
-The following commands were used to compile the source to a memprof instrumented
-executable and collect a raw binary format profile. Since the profile contains
-virtual addresses for the callstack, we do not expect the raw binary profile to
-be deterministic. The summary should be deterministic apart from changes to
-the shared libraries linked in which could change the number of segments
+Since the profile contains virtual addresses for the callstack,
+we do not expect the raw binary profile to be deterministic. The
+summary should be deterministic apart from changes to the shared
+libraries linked in which could change the number of segments
 recorded.
 
-```
-clang -fuse-ld=lld -Wl,--no-rosegment -gmlt -fdebug-info-for-profiling \
-  -fmemory-profile -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer \
-  -fno-optimize-sibling-calls -m64 -Wl,-build-id -pie \
-  source.c -o pic.memprofexe
-
-env MEMPROF_OPTIONS=log_path=stdout ./pic.memprofexe > pic.memprofraw
-```
-
+To update the inputs used below run Inputs/update_memprof_inputs.sh /path/to/updated/clang
 RUN: not llvm-profdata show --memory %p/Inputs/pic.memprofraw --profiled-binary %p/Inputs/pic.memprofexe -o - 2>&1 | FileCheck %s
+
 CHECK: Unsupported position independent code
Index: llvm/test/tools/llvm-profdata/memprof-multi.test
===
--- llvm/test/tools/llvm-profdata/memprof-multi.test
+++ llvm/test/tools/llvm-profdata/memprof-multi.test
@@ -1,39 +1,6 @@
 REQUIRES: x86_64-linux
 
-The input raw profile test has been generated from the following source code:
-
-```
-#include 
-#include 
-#include 
-int main(int argc, char **argv) {
-  char *x = (char *)malloc(10);
-  memset(x, 0, 10);
-  free(x);
-  __memprof_profile_dump();
-  x = (char *)malloc(10);
-  memset(x, 0, 10);
-  free(x);
-  return 0;
-}
-```
-
-The following commands were used to compile the source to a memprof instrumented
-executable and collect a raw binary format profile. Since the profile contains
-virtual addresses for the callstack, we do not expect the raw binary profile to
-be deterministic. The summary should be deterministic apart from changes to
-the shared libraries linked in which could change the number of segments
-recorded.
-
-```
-clang -fuse-ld=lld -Wl,--no-rosegment -gmlt -fdebug-info-for-profiling \
-  -fmemory-profile -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer \
-  -fno-optimize-sibling-calls -m64 -Wl,-build-id -no-pie \
-  source.c -o multi.memprofexe
-
-env MEMPROF_OPTIONS=log_path=stdout 

[PATCH] D145644: [memprof] Add scripts to automate testdata regeneration.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish added a comment.

I'll go ahead and push this. @dblaikie Let me know if you have any comments and 
I'll follow up separately. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145644

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

[PATCH] D145644: [memprof] Add scripts to automate testdata regeneration.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish marked an inline comment as done.
snehasish added a comment.

In D145644#4182239 , @tejohnson wrote:

> Generally lgtm, but why did the raw profiles change size from what is 
> currently committed?

The reduction in size of the raw profiles is due to D145528 
 where we moved the initialization flags. The 
older raw profiles include e.g. 4 entries for the basic test case including 
calls to malloc from `Symbolizer::LateInitialize`. These two entries were 
pruned by the filtering logic in RawMemProfReader but contribute to the 
original raw file size. After D145528  we 
only have two entries in the generated raw profile. Verified by making a local 
change to disable filtering and ran `llvm-profdata show --memory` on the older 
profiles.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145644

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

[PATCH] D145644: [memprof] Add scripts to automate testdata regeneration.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish updated this revision to Diff 503859.
snehasish added a comment.

Update the script in test/tool/llvm-profdata/Inputs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145644

Files:
  clang/test/CodeGen/Inputs/memprof.exe
  clang/test/CodeGen/Inputs/memprof.memprofraw
  clang/test/CodeGen/Inputs/update_memprof_inputs.sh
  clang/test/CodeGen/memprof.cpp
  llvm/test/Transforms/PGOProfile/Inputs/memprof.exe
  llvm/test/Transforms/PGOProfile/Inputs/memprof.memprofraw
  llvm/test/Transforms/PGOProfile/Inputs/memprof_pgo.profraw
  llvm/test/Transforms/PGOProfile/Inputs/update_memprof_inputs.sh
  llvm/test/Transforms/PGOProfile/memprof.ll
  llvm/test/tools/llvm-profdata/Inputs/basic.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/basic.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/inline.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/inline.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/memprof-inline.exe
  llvm/test/tools/llvm-profdata/Inputs/multi.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/multi.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/pic.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/pic.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/update_memprof_inputs.sh
  llvm/test/tools/llvm-profdata/memprof-basic.test
  llvm/test/tools/llvm-profdata/memprof-inline.test
  llvm/test/tools/llvm-profdata/memprof-merge.test
  llvm/test/tools/llvm-profdata/memprof-multi.test
  llvm/test/tools/llvm-profdata/memprof-pic.test

Index: llvm/test/tools/llvm-profdata/memprof-pic.test
===
--- llvm/test/tools/llvm-profdata/memprof-pic.test
+++ llvm/test/tools/llvm-profdata/memprof-pic.test
@@ -1,40 +1,12 @@
 REQUIRES: x86_64-linux
 
-This test ensures that llvm-profdata fails with a descriptive error message
-when invoked on a memprof profiled binary which was built with position
-independent code.
-
-The input raw profile test has been generated from the following source code:
-
-```
-#include 
-#include 
-int main(int argc, char **argv) {
-  char *x = (char *)malloc(10);
-  memset(x, 0, 10);
-  free(x);
-  x = (char *)malloc(10);
-  memset(x, 0, 10);
-  free(x);
-  return 0;
-}
-```
-
-The following commands were used to compile the source to a memprof instrumented
-executable and collect a raw binary format profile. Since the profile contains
-virtual addresses for the callstack, we do not expect the raw binary profile to
-be deterministic. The summary should be deterministic apart from changes to
-the shared libraries linked in which could change the number of segments
+Since the profile contains virtual addresses for the callstack,
+we do not expect the raw binary profile to be deterministic. The
+summary should be deterministic apart from changes to the shared
+libraries linked in which could change the number of segments
 recorded.
 
-```
-clang -fuse-ld=lld -Wl,--no-rosegment -gmlt -fdebug-info-for-profiling \
-  -fmemory-profile -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer \
-  -fno-optimize-sibling-calls -m64 -Wl,-build-id -pie \
-  source.c -o pic.memprofexe
-
-env MEMPROF_OPTIONS=log_path=stdout ./pic.memprofexe > pic.memprofraw
-```
-
+To update the inputs used below run Inputs/update_memprof_inputs.sh /path/to/updated/clang
 RUN: not llvm-profdata show --memory %p/Inputs/pic.memprofraw --profiled-binary %p/Inputs/pic.memprofexe -o - 2>&1 | FileCheck %s
+
 CHECK: Unsupported position independent code
Index: llvm/test/tools/llvm-profdata/memprof-multi.test
===
--- llvm/test/tools/llvm-profdata/memprof-multi.test
+++ llvm/test/tools/llvm-profdata/memprof-multi.test
@@ -1,39 +1,6 @@
 REQUIRES: x86_64-linux
 
-The input raw profile test has been generated from the following source code:
-
-```
-#include 
-#include 
-#include 
-int main(int argc, char **argv) {
-  char *x = (char *)malloc(10);
-  memset(x, 0, 10);
-  free(x);
-  __memprof_profile_dump();
-  x = (char *)malloc(10);
-  memset(x, 0, 10);
-  free(x);
-  return 0;
-}
-```
-
-The following commands were used to compile the source to a memprof instrumented
-executable and collect a raw binary format profile. Since the profile contains
-virtual addresses for the callstack, we do not expect the raw binary profile to
-be deterministic. The summary should be deterministic apart from changes to
-the shared libraries linked in which could change the number of segments
-recorded.
-
-```
-clang -fuse-ld=lld -Wl,--no-rosegment -gmlt -fdebug-info-for-profiling \
-  -fmemory-profile -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer \
-  -fno-optimize-sibling-calls -m64 -Wl,-build-id -no-pie \
-  source.c -o multi.memprofexe
-
-env MEMPROF_OPTIONS=log_path=stdout ./multi.memprofexe > multi.memprofraw
-```
-
+To update the inputs used below run Inputs/update_memprof_inputs.sh 

[PATCH] D145644: [memprof] Add scripts to automate testdata regeneration.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish added inline comments.



Comment at: llvm/test/Transforms/PGOProfile/Inputs/update_memprof_inputs.sh:63
+  delete[] f;
+
+  // Loop ensures the two calls to recurse have stack contexts that only differ

@tejohnson Turns out the LLVM IR matching issue was due to a difference in the 
code from which the IR was generated. The one in the comments has a line offset 
of 30 from `main` to the callsite of `recurse` whereas the memprof.ll IR 
expects a line offset of 31. Perhaps a comment or blank line was omitted? I 
added a blank line here and added a comment in the update script too. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145644

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

[PATCH] D145644: [memprof] Add scripts to automate testdata regeneration.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish updated this revision to Diff 503850.
snehasish added a comment.

Update the script for Transforms/PGOProfile/memprof.ll.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145644

Files:
  clang/test/CodeGen/Inputs/memprof.exe
  clang/test/CodeGen/Inputs/memprof.memprofraw
  clang/test/CodeGen/Inputs/update_memprof_inputs.sh
  clang/test/CodeGen/memprof.cpp
  llvm/test/Transforms/PGOProfile/Inputs/memprof.exe
  llvm/test/Transforms/PGOProfile/Inputs/memprof.memprofraw
  llvm/test/Transforms/PGOProfile/Inputs/memprof_pgo.profraw
  llvm/test/Transforms/PGOProfile/Inputs/update_memprof_inputs.sh
  llvm/test/Transforms/PGOProfile/memprof.ll
  llvm/test/tools/llvm-profdata/Inputs/basic.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/basic.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/inline.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/inline.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/memprof-inline.exe
  llvm/test/tools/llvm-profdata/Inputs/multi.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/multi.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/pic.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/pic.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/update_memprof_inputs.sh
  llvm/test/tools/llvm-profdata/memprof-basic.test
  llvm/test/tools/llvm-profdata/memprof-inline.test
  llvm/test/tools/llvm-profdata/memprof-merge.test
  llvm/test/tools/llvm-profdata/memprof-multi.test
  llvm/test/tools/llvm-profdata/memprof-pic.test

Index: llvm/test/tools/llvm-profdata/memprof-pic.test
===
--- llvm/test/tools/llvm-profdata/memprof-pic.test
+++ llvm/test/tools/llvm-profdata/memprof-pic.test
@@ -1,40 +1,12 @@
 REQUIRES: x86_64-linux
 
-This test ensures that llvm-profdata fails with a descriptive error message
-when invoked on a memprof profiled binary which was built with position
-independent code.
-
-The input raw profile test has been generated from the following source code:
-
-```
-#include 
-#include 
-int main(int argc, char **argv) {
-  char *x = (char *)malloc(10);
-  memset(x, 0, 10);
-  free(x);
-  x = (char *)malloc(10);
-  memset(x, 0, 10);
-  free(x);
-  return 0;
-}
-```
-
-The following commands were used to compile the source to a memprof instrumented
-executable and collect a raw binary format profile. Since the profile contains
-virtual addresses for the callstack, we do not expect the raw binary profile to
-be deterministic. The summary should be deterministic apart from changes to
-the shared libraries linked in which could change the number of segments
+Since the profile contains virtual addresses for the callstack,
+we do not expect the raw binary profile to be deterministic. The
+summary should be deterministic apart from changes to the shared
+libraries linked in which could change the number of segments
 recorded.
 
-```
-clang -fuse-ld=lld -Wl,--no-rosegment -gmlt -fdebug-info-for-profiling \
-  -fmemory-profile -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer \
-  -fno-optimize-sibling-calls -m64 -Wl,-build-id -pie \
-  source.c -o pic.memprofexe
-
-env MEMPROF_OPTIONS=log_path=stdout ./pic.memprofexe > pic.memprofraw
-```
-
+To update the inputs used below run Inputs/update_memprof_inputs.sh /path/to/updated/clang
 RUN: not llvm-profdata show --memory %p/Inputs/pic.memprofraw --profiled-binary %p/Inputs/pic.memprofexe -o - 2>&1 | FileCheck %s
+
 CHECK: Unsupported position independent code
Index: llvm/test/tools/llvm-profdata/memprof-multi.test
===
--- llvm/test/tools/llvm-profdata/memprof-multi.test
+++ llvm/test/tools/llvm-profdata/memprof-multi.test
@@ -1,39 +1,6 @@
 REQUIRES: x86_64-linux
 
-The input raw profile test has been generated from the following source code:
-
-```
-#include 
-#include 
-#include 
-int main(int argc, char **argv) {
-  char *x = (char *)malloc(10);
-  memset(x, 0, 10);
-  free(x);
-  __memprof_profile_dump();
-  x = (char *)malloc(10);
-  memset(x, 0, 10);
-  free(x);
-  return 0;
-}
-```
-
-The following commands were used to compile the source to a memprof instrumented
-executable and collect a raw binary format profile. Since the profile contains
-virtual addresses for the callstack, we do not expect the raw binary profile to
-be deterministic. The summary should be deterministic apart from changes to
-the shared libraries linked in which could change the number of segments
-recorded.
-
-```
-clang -fuse-ld=lld -Wl,--no-rosegment -gmlt -fdebug-info-for-profiling \
-  -fmemory-profile -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer \
-  -fno-optimize-sibling-calls -m64 -Wl,-build-id -no-pie \
-  source.c -o multi.memprofexe
-
-env MEMPROF_OPTIONS=log_path=stdout ./multi.memprofexe > multi.memprofraw
-```
-
+To update the inputs used below run Inputs/update_memprof_inputs.sh 

[PATCH] D145644: [memprof] Add scripts to automate testdata regeneration.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish created this revision.
snehasish added reviewers: tejohnson, dblaikie.
Herald added a subscriber: wenlei.
Herald added a project: All.
snehasish requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

The memprof profiles and binaries need to be updated in case of version
updates. This change adds three scripts for llvm-profdata, clang and
llvm tests where memprof profiles are used as inputs. Also update the
tests, profiles and binaries in this change. Change based on the review
suggestions in D145023 .


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D145644

Files:
  clang/test/CodeGen/Inputs/memprof.exe
  clang/test/CodeGen/Inputs/memprof.memprofraw
  clang/test/CodeGen/Inputs/update_memprof_inputs.sh
  clang/test/CodeGen/memprof.cpp
  llvm/test/Transforms/PGOProfile/Inputs/memprof.exe
  llvm/test/Transforms/PGOProfile/Inputs/memprof.memprofraw
  llvm/test/Transforms/PGOProfile/Inputs/update_memprof_inputs.sh
  llvm/test/Transforms/PGOProfile/memprof.ll
  llvm/test/tools/llvm-profdata/Inputs/basic.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/basic.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/inline.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/inline.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/memprof-inline.exe
  llvm/test/tools/llvm-profdata/Inputs/multi.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/multi.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/pic.memprofexe
  llvm/test/tools/llvm-profdata/Inputs/pic.memprofraw
  llvm/test/tools/llvm-profdata/Inputs/update_memprof_inputs.sh
  llvm/test/tools/llvm-profdata/memprof-basic.test
  llvm/test/tools/llvm-profdata/memprof-inline.test
  llvm/test/tools/llvm-profdata/memprof-merge.test
  llvm/test/tools/llvm-profdata/memprof-multi.test
  llvm/test/tools/llvm-profdata/memprof-pic.test

Index: llvm/test/tools/llvm-profdata/memprof-pic.test
===
--- llvm/test/tools/llvm-profdata/memprof-pic.test
+++ llvm/test/tools/llvm-profdata/memprof-pic.test
@@ -1,40 +1,12 @@
 REQUIRES: x86_64-linux
 
-This test ensures that llvm-profdata fails with a descriptive error message
-when invoked on a memprof profiled binary which was built with position
-independent code.
-
-The input raw profile test has been generated from the following source code:
-
-```
-#include 
-#include 
-int main(int argc, char **argv) {
-  char *x = (char *)malloc(10);
-  memset(x, 0, 10);
-  free(x);
-  x = (char *)malloc(10);
-  memset(x, 0, 10);
-  free(x);
-  return 0;
-}
-```
-
-The following commands were used to compile the source to a memprof instrumented
-executable and collect a raw binary format profile. Since the profile contains
-virtual addresses for the callstack, we do not expect the raw binary profile to
-be deterministic. The summary should be deterministic apart from changes to
-the shared libraries linked in which could change the number of segments
+Since the profile contains virtual addresses for the callstack,
+we do not expect the raw binary profile to be deterministic. The
+summary should be deterministic apart from changes to the shared
+libraries linked in which could change the number of segments
 recorded.
 
-```
-clang -fuse-ld=lld -Wl,--no-rosegment -gmlt -fdebug-info-for-profiling \
-  -fmemory-profile -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer \
-  -fno-optimize-sibling-calls -m64 -Wl,-build-id -pie \
-  source.c -o pic.memprofexe
-
-env MEMPROF_OPTIONS=log_path=stdout ./pic.memprofexe > pic.memprofraw
-```
-
+To update the inputs used below run Inputs/update_memprof_inputs.sh /path/to/updated/clang
 RUN: not llvm-profdata show --memory %p/Inputs/pic.memprofraw --profiled-binary %p/Inputs/pic.memprofexe -o - 2>&1 | FileCheck %s
+
 CHECK: Unsupported position independent code
Index: llvm/test/tools/llvm-profdata/memprof-multi.test
===
--- llvm/test/tools/llvm-profdata/memprof-multi.test
+++ llvm/test/tools/llvm-profdata/memprof-multi.test
@@ -1,39 +1,6 @@
 REQUIRES: x86_64-linux
 
-The input raw profile test has been generated from the following source code:
-
-```
-#include 
-#include 
-#include 
-int main(int argc, char **argv) {
-  char *x = (char *)malloc(10);
-  memset(x, 0, 10);
-  free(x);
-  __memprof_profile_dump();
-  x = (char *)malloc(10);
-  memset(x, 0, 10);
-  free(x);
-  return 0;
-}
-```
-
-The following commands were used to compile the source to a memprof instrumented
-executable and collect a raw binary format profile. Since the profile contains
-virtual addresses for the callstack, we do not expect the raw binary profile to
-be deterministic. The summary should be deterministic apart from changes to
-the shared libraries linked in which could change the number of segments
-recorded.
-
-```
-clang -fuse-ld=lld -Wl,--no-rosegment -gmlt 

[PATCH] D141558: [MemProf] Collect access density statistics during profiling

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish accepted this revision.
snehasish added a comment.
This revision is now accepted and ready to land.

lgtm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141558

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

[PATCH] D141558: [MemProf] Collect access density statistics during profiling

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish added a comment.

Thanks for updating the tests.




Comment at: llvm/include/llvm/ProfileData/MemProfData.inc:35
 // The version number of the raw binary format.
 #define MEMPROF_RAW_VERSION 1ULL
 

Can you increment the version here? Then we won't accidentally read the old 
data with the new reader which expects more data per MemInfoBlock.

Also update the other copy of MemProfData.inc.



Comment at: llvm/test/tools/llvm-profdata/memprof-multi.test:44
 CHECK-NEXT:Version: 1
-CHECK-NEXT:NumSegments: 9
+CHECK-NEXT:NumSegments: 7
 CHECK-NEXT:NumMibInfo: 2

Use the same regex for NumSegments here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141558

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

[PATCH] D128142: [MemProf] Memprof profile matching and annotation

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish accepted this revision.
snehasish added a comment.
This revision is now accepted and ready to land.

lgtm




Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1276
+  if (Error E = MemProfResult.takeError()) {
+handleAllErrors(std::move(E), [&](const InstrProfError ) {
+  auto Err = IPE.get();

tejohnson wrote:
> snehasish wrote:
> > Consider defining the lambda outside above the condition to reduce 
> > indentation. IMO it will be  a little easier to follow if it wasn't inlined 
> > into the if statement itself.
> I could do this, but as is it mirrors the structure of the similar handling 
> in readCounters, which has some advantages. wdyt?
I wasn't a big fan of the existing structure in readCounters but I didn't want 
to ask you to change the other code. Let's leave it as is for now. 



Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1360
+  // If leaf was found in a map, iterators pointing to its location in both
+  // of the maps (it may only exist in one).
+  std::map>::iterator

tejohnson wrote:
> snehasish wrote:
> > Can you add an assert for this?
> In this case "may only" meant "might only", not "may only at most". So I 
> can't assert on anything. This can happen for example if we have a location 
> that corresponds to both an allocation call and another callsite (I've seen 
> this periodically, and can reproduce e.g. with a macro). We would need to use 
> discriminators more widely to better distinguish them in that case (with the 
> handling here we will only match to the allocation call for now - edit: a 
> slight change noted further below ensures this is the case). Will change 
> /may/might/ and add a note.
Thanks for the explanation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128142

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

[PATCH] D128142: [MemProf] Memprof profile matching and annotation

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish added inline comments.
Herald added a subscriber: mingmingl.



Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1249
+void addCallStack(CallStackTrie , const AllocationInfo *AllocInfo) {
+  auto AllocType = getAllocType(AllocInfo->Info.getMaxAccessCount(),
+AllocInfo->Info.getMinSize(),

Prefer moving this code after the loop, close to where AllocType is used.



Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1252
+AllocInfo->Info.getMinLifetime());
+  SmallVector StackIds;
+  std::set StackHashSet;

I think if you use an llvm::SetVector here instead then you don't need the 
StackHashSet std::set below. CallstackTrie::addCallstack already accepts an 
ArrayRef so it won't need to change if we use a SetVector.



Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1253
+  SmallVector StackIds;
+  std::set StackHashSet;
+  for (auto StackFrame : AllocInfo->CallStack) {

nit: It doesn't look like we #include  in this file so we are probably 
relying on having it transitively being included from somewhere.



Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1276
+  if (Error E = MemProfResult.takeError()) {
+handleAllErrors(std::move(E), [&](const InstrProfError ) {
+  auto Err = IPE.get();

Consider defining the lambda outside above the condition to reduce indentation. 
IMO it will be  a little easier to follow if it wasn't inlined into the if 
statement itself.



Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1297
+
+  std::string Msg = IPE.message() + std::string(" ") + F.getName().str() +
+std::string(" Hash = ") +

Is an llvm::Twine a better choice here instead of std::string? I guess it 
doesn't matter much in error handling code.  



Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1313
+  std::map *, unsigned>>>
+  LocHashToCallSites;
+  const auto MemProfRec = std::move(MemProfResult.get());

LocHashToCallSiteFrame to indicate the value in the map corresponds to an 
individual frame?



Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1319
+// of call stack frames.
+auto StackId = computeStackId(AI.CallStack[0]);
+LocHashToAllocInfo[StackId].insert();

Not using auto over here would be helpful to know that we are indexing into the 
map below using an uint64_t. Same below.



Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1330
+  // Once we find this function, we can stop recording.
+  if (StackFrame.Function == FuncGUID)
+break;

Should we assert that it was actually found?



Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1360
+  // If leaf was found in a map, iterators pointing to its location in both
+  // of the maps (it may only exist in one).
+  std::map>::iterator

Can you add an assert for this?



Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1365
+unsigned>>>::iterator 
CallSitesIter;
+  for (const DILocation *DIL = I.getDebugLoc(); DIL;
+   DIL = DIL->getInlinedAt()) {

`DIL != nullptr` is a little easier to follow.



Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1397
+  // be non-zero.
+  auto StackFrameIncludesInlinedCallStack =
+  [](ArrayRef ProfileCallStack,

Prefer moving this to a static helper method to reduce the size of the loop 
body, reduce indentation for this logic and make it more readable overall. 
Probably creating an functor object on the stack for each instruction that we 
process is not efficient either.



Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1414
+
+  // First add !memprof metadata from allocation info, if we found the
+  // instruction's leaf location in that map, and if the rest of the

"First add !memprof metadata  ..." -- the ordering of the if-else condition 
isn't necessary though since only one of the iters can be non-null? We could 
rewrite the else condition first to reduce the complexity here a bit. Eg --

```
if (CallSitesIter != LocHashToCallSites.end()) {
  ...
  continue
}

// Flip the conditions here
if (!isNewLikeFn() || AllocInfoIter == LocHashToAllocInfo.end()) {
   continue
}

CallStackTrie AllocTrie;
...
```



Comment at: llvm/test/Transforms/PGOProfile/memprof.ll:82
+;; memprof.cc -o pgo.exe -fprofile-generate=.
+;; $ pgo.exe
+;; $ mv 

[PATCH] D128142: [MemProf] Memprof profile matching and annotation

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish added a comment.

I'm still going through PGOInstrumentation.cpp ...




Comment at: clang/test/CodeGen/memprof.cpp:15
+// # Collect memory profile:
+// $ clang++ -fuse-ld=lld -Wl,-no-pie -Wl,--no-rosegment -gmlt \
+//  -fdebug-info-for-profiling -mno-omit-leaf-frame-pointer \

Just `-no-pie` is better (details: https://reviews.llvm.org/rG103b28902fd6)



Comment at: llvm/include/llvm/Analysis/MemoryProfileInfo.h:1
+//===- llvm/Analysis/MemoryProfileInfo.h - memory profile info ---*- C++ 
-*-==//
+//

Can you split out the memory profile info parts into a separate patch? Also I 
think the interface is simple enough to be able to unit test. Something set up 
like [1] would be great. Then we can call addCallstack with different 
annotations followed by buildAndAttachMIBMetaData followed by checking the 
metadata annotated on the call inst(s). What do you think?

[1] 
https://github.com/llvm/llvm-project/blob/3f8e4169c1c390fd086658c1e51983ee61bff9bc/llvm/unittests/Analysis/FunctionPropertiesAnalysisTest.cpp#L71



Comment at: llvm/include/llvm/Analysis/MemoryProfileInfo.h:40
+
+// Class to build a trie of call stack contexts for a particular profiled
+// allocation call, along with their associated allocation types.

Should this be a doxygen comment block with `///`?



Comment at: llvm/include/llvm/ProfileData/InstrProfReader.h:333
 
+  bool hasMemoryProfile() const override {
+return (Version & VARIANT_MASK_MEMPROF) != 0;

The RawInstrProfReader shouldn't have the memprof mask set. We have a separate  
raw binary format which is independent. So this should always return false. 
Also maybe add a comment to document the fact?



Comment at: llvm/lib/Analysis/MemoryProfileInfo.cpp:19
+
+#define DEBUG_TYPE "memory-profile-info"
+

We use MemoryProfile and MemProf interchangeably. Does it make sense to pick 
one and make it consistent throughout? Here for eg. the flags begin with 
"memprof-*" but the debug type is "memory-profile-*".



Comment at: llvm/lib/Analysis/MemoryProfileInfo.cpp:102
+assert(AllocStackId == StackId);
+Alloc->AllocTypes |= (uint8_t)AllocType;
+  } else {

nit: prefer static_cast here and elsewhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128142

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

[PATCH] D115393: [InstrProf][NFC] Refactor Profile kind into a bitset enum.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish added inline comments.



Comment at: llvm/include/llvm/ProfileData/InstrProfReader.h:495
 
+  InstrProfKind getProfileKind() const override {
+InstrProfKind ProfileKind = InstrProfKind::Unknown;

mtrofin wrote:
> This looks a lot like line 290, can it be refactored (or am I missing 
> something?)
Since these were in a header but spread across different inherited classes of 
the same base I moved these to the .cpp and refactored it into a common static 
function in https://reviews.llvm.org/D118656. Thanks!



Comment at: llvm/lib/ProfileData/InstrProfWriter.cpp:337
   uint64_t CSSummarySize = 0;
-  if (ProfileKind == PF_IRLevelWithCS) {
+  if (static_cast(ProfileKind & InstrProfKind::CS)) {
 CSSummaryOffset = OS.tell();

mtrofin wrote:
> consider adding helper APIs for test/set?
I'm not sure it will help much but I'll take a look in the patch to rename the 
enum types to more descriptive names.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115393

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

[PATCH] D115393: [InstrProf][NFC] Refactor Profile kind into a bitset enum.

[Snehasish Kumar via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG13d89477be56: [InstrProf][NFC] Refactor Profile kind into a 
bitset enum. (authored by snehasish).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115393

Files:
  llvm/include/llvm/ProfileData/InstrProf.h
  llvm/include/llvm/ProfileData/InstrProfReader.h
  llvm/include/llvm/ProfileData/InstrProfWriter.h
  llvm/lib/ProfileData/InstrProfReader.cpp
  llvm/lib/ProfileData/InstrProfWriter.cpp
  llvm/tools/llvm-profdata/llvm-profdata.cpp

Index: llvm/tools/llvm-profdata/llvm-profdata.cpp
===
--- llvm/tools/llvm-profdata/llvm-profdata.cpp
+++ llvm/tools/llvm-profdata/llvm-profdata.cpp
@@ -255,9 +255,7 @@
   }
 
   auto Reader = std::move(ReaderOrErr.get());
-  bool IsIRProfile = Reader->isIRLevelProfile();
-  bool HasCSIRProfile = Reader->hasCSIRLevelProfile();
-  if (Error E = WC->Writer.setIsIRLevelProfile(IsIRProfile, HasCSIRProfile)) {
+  if (Error E = WC->Writer.mergeProfileKind(Reader->getProfileKind())) {
 consumeError(std::move(E));
 WC->Errors.emplace_back(
 make_error(
@@ -266,7 +264,6 @@
 Filename);
 return;
   }
-  WC->Writer.setInstrEntryBBEnabled(Reader->instrEntryBBEnabled());
 
   for (auto  : *Reader) {
 if (Remapper)
Index: llvm/lib/ProfileData/InstrProfWriter.cpp
===
--- llvm/lib/ProfileData/InstrProfWriter.cpp
+++ llvm/lib/ProfileData/InstrProfWriter.cpp
@@ -166,9 +166,8 @@
 
 } // end namespace llvm
 
-InstrProfWriter::InstrProfWriter(bool Sparse, bool InstrEntryBBEnabled)
-: Sparse(Sparse), InstrEntryBBEnabled(InstrEntryBBEnabled),
-  InfoObj(new InstrProfRecordWriterTrait()) {}
+InstrProfWriter::InstrProfWriter(bool Sparse)
+: Sparse(Sparse), InfoObj(new InstrProfRecordWriterTrait()) {}
 
 InstrProfWriter::~InstrProfWriter() { delete InfoObj; }
 
@@ -303,13 +302,11 @@
   IndexedInstrProf::Header Header;
   Header.Magic = IndexedInstrProf::Magic;
   Header.Version = IndexedInstrProf::ProfVersion::CurrentVersion;
-  if (ProfileKind == PF_IRLevel)
-Header.Version |= VARIANT_MASK_IR_PROF;
-  if (ProfileKind == PF_IRLevelWithCS) {
+  if (static_cast(ProfileKind & InstrProfKind::IR))
 Header.Version |= VARIANT_MASK_IR_PROF;
+  if (static_cast(ProfileKind & InstrProfKind::CS))
 Header.Version |= VARIANT_MASK_CSIR_PROF;
-  }
-  if (InstrEntryBBEnabled)
+  if (static_cast(ProfileKind & InstrProfKind::BB))
 Header.Version |= VARIANT_MASK_INSTR_ENTRY;
 
   Header.Unused = 0;
@@ -337,7 +334,7 @@
 OS.write(0);
   uint64_t CSSummaryOffset = 0;
   uint64_t CSSummarySize = 0;
-  if (ProfileKind == PF_IRLevelWithCS) {
+  if (static_cast(ProfileKind & InstrProfKind::CS)) {
 CSSummaryOffset = OS.tell();
 CSSummarySize = SummarySize / sizeof(uint64_t);
 for (unsigned I = 0; I < CSSummarySize; I++)
@@ -358,7 +355,7 @@
 
   // For Context Sensitive summary.
   std::unique_ptr TheCSSummary = nullptr;
-  if (ProfileKind == PF_IRLevelWithCS) {
+  if (static_cast(ProfileKind & InstrProfKind::CS)) {
 TheCSSummary = IndexedInstrProf::allocSummary(SummarySize);
 std::unique_ptr CSPS = CSISB.getSummary();
 setSummary(TheCSSummary.get(), *CSPS);
@@ -470,11 +467,13 @@
 }
 
 Error InstrProfWriter::writeText(raw_fd_ostream ) {
-  if (ProfileKind == PF_IRLevel)
-OS << "# IR level Instrumentation Flag\n:ir\n";
-  else if (ProfileKind == PF_IRLevelWithCS)
+  // Check CS first since it implies an IR level profile.
+  if (static_cast(ProfileKind & InstrProfKind::CS))
 OS << "# CSIR level Instrumentation Flag\n:csir\n";
-  if (InstrEntryBBEnabled)
+  else if (static_cast(ProfileKind & InstrProfKind::IR))
+OS << "# IR level Instrumentation Flag\n:ir\n";
+
+  if (static_cast(ProfileKind & InstrProfKind::BB))
 OS << "# Always instrument the function entry block\n:entry_first\n";
   InstrProfSymtab Symtab;
 
Index: llvm/lib/ProfileData/InstrProfReader.cpp
===
--- llvm/lib/ProfileData/InstrProfReader.cpp
+++ llvm/lib/ProfileData/InstrProfReader.cpp
@@ -154,30 +154,24 @@
 // with a leading ':' will be reported an error format.
 Error TextInstrProfReader::readHeader() {
   Symtab.reset(new InstrProfSymtab());
-  bool IsIRInstr = false;
-  bool IsEntryFirst = false;
-  bool IsCS = false;
 
   while (Line->startswith(":")) {
 StringRef Str = Line->substr(1);
 if (Str.equals_insensitive("ir"))
-  IsIRInstr = true;
+  ProfileKind |= InstrProfKind::IR;
 else if (Str.equals_insensitive("fe"))
-  IsIRInstr = false;
+  ProfileKind |= InstrProfKind::FE;
 else if (Str.equals_insensitive("csir")) {
-  IsIRInstr = true;
-  IsCS = true;
+  ProfileKind |= InstrProfKind::IR;
+  

[PATCH] D115393: [InstrProf][NFC] Refactor Profile kind into a bitset enum.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish updated this revision to Diff 403746.
snehasish added a comment.

Rebase.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115393

Files:
  llvm/include/llvm/ProfileData/InstrProf.h
  llvm/include/llvm/ProfileData/InstrProfReader.h
  llvm/include/llvm/ProfileData/InstrProfWriter.h
  llvm/lib/ProfileData/InstrProfReader.cpp
  llvm/lib/ProfileData/InstrProfWriter.cpp
  llvm/tools/llvm-profdata/llvm-profdata.cpp

Index: llvm/tools/llvm-profdata/llvm-profdata.cpp
===
--- llvm/tools/llvm-profdata/llvm-profdata.cpp
+++ llvm/tools/llvm-profdata/llvm-profdata.cpp
@@ -255,9 +255,7 @@
   }
 
   auto Reader = std::move(ReaderOrErr.get());
-  bool IsIRProfile = Reader->isIRLevelProfile();
-  bool HasCSIRProfile = Reader->hasCSIRLevelProfile();
-  if (Error E = WC->Writer.setIsIRLevelProfile(IsIRProfile, HasCSIRProfile)) {
+  if (Error E = WC->Writer.mergeProfileKind(Reader->getProfileKind())) {
 consumeError(std::move(E));
 WC->Errors.emplace_back(
 make_error(
@@ -266,7 +264,6 @@
 Filename);
 return;
   }
-  WC->Writer.setInstrEntryBBEnabled(Reader->instrEntryBBEnabled());
 
   for (auto  : *Reader) {
 if (Remapper)
Index: llvm/lib/ProfileData/InstrProfWriter.cpp
===
--- llvm/lib/ProfileData/InstrProfWriter.cpp
+++ llvm/lib/ProfileData/InstrProfWriter.cpp
@@ -166,9 +166,8 @@
 
 } // end namespace llvm
 
-InstrProfWriter::InstrProfWriter(bool Sparse, bool InstrEntryBBEnabled)
-: Sparse(Sparse), InstrEntryBBEnabled(InstrEntryBBEnabled),
-  InfoObj(new InstrProfRecordWriterTrait()) {}
+InstrProfWriter::InstrProfWriter(bool Sparse)
+: Sparse(Sparse), InfoObj(new InstrProfRecordWriterTrait()) {}
 
 InstrProfWriter::~InstrProfWriter() { delete InfoObj; }
 
@@ -303,13 +302,11 @@
   IndexedInstrProf::Header Header;
   Header.Magic = IndexedInstrProf::Magic;
   Header.Version = IndexedInstrProf::ProfVersion::CurrentVersion;
-  if (ProfileKind == PF_IRLevel)
-Header.Version |= VARIANT_MASK_IR_PROF;
-  if (ProfileKind == PF_IRLevelWithCS) {
+  if (static_cast(ProfileKind & InstrProfKind::IR))
 Header.Version |= VARIANT_MASK_IR_PROF;
+  if (static_cast(ProfileKind & InstrProfKind::CS))
 Header.Version |= VARIANT_MASK_CSIR_PROF;
-  }
-  if (InstrEntryBBEnabled)
+  if (static_cast(ProfileKind & InstrProfKind::BB))
 Header.Version |= VARIANT_MASK_INSTR_ENTRY;
 
   Header.Unused = 0;
@@ -337,7 +334,7 @@
 OS.write(0);
   uint64_t CSSummaryOffset = 0;
   uint64_t CSSummarySize = 0;
-  if (ProfileKind == PF_IRLevelWithCS) {
+  if (static_cast(ProfileKind & InstrProfKind::CS)) {
 CSSummaryOffset = OS.tell();
 CSSummarySize = SummarySize / sizeof(uint64_t);
 for (unsigned I = 0; I < CSSummarySize; I++)
@@ -358,7 +355,7 @@
 
   // For Context Sensitive summary.
   std::unique_ptr TheCSSummary = nullptr;
-  if (ProfileKind == PF_IRLevelWithCS) {
+  if (static_cast(ProfileKind & InstrProfKind::CS)) {
 TheCSSummary = IndexedInstrProf::allocSummary(SummarySize);
 std::unique_ptr CSPS = CSISB.getSummary();
 setSummary(TheCSSummary.get(), *CSPS);
@@ -470,11 +467,13 @@
 }
 
 Error InstrProfWriter::writeText(raw_fd_ostream ) {
-  if (ProfileKind == PF_IRLevel)
-OS << "# IR level Instrumentation Flag\n:ir\n";
-  else if (ProfileKind == PF_IRLevelWithCS)
+  // Check CS first since it implies an IR level profile.
+  if (static_cast(ProfileKind & InstrProfKind::CS))
 OS << "# CSIR level Instrumentation Flag\n:csir\n";
-  if (InstrEntryBBEnabled)
+  else if (static_cast(ProfileKind & InstrProfKind::IR))
+OS << "# IR level Instrumentation Flag\n:ir\n";
+
+  if (static_cast(ProfileKind & InstrProfKind::BB))
 OS << "# Always instrument the function entry block\n:entry_first\n";
   InstrProfSymtab Symtab;
 
Index: llvm/lib/ProfileData/InstrProfReader.cpp
===
--- llvm/lib/ProfileData/InstrProfReader.cpp
+++ llvm/lib/ProfileData/InstrProfReader.cpp
@@ -154,30 +154,24 @@
 // with a leading ':' will be reported an error format.
 Error TextInstrProfReader::readHeader() {
   Symtab.reset(new InstrProfSymtab());
-  bool IsIRInstr = false;
-  bool IsEntryFirst = false;
-  bool IsCS = false;
 
   while (Line->startswith(":")) {
 StringRef Str = Line->substr(1);
 if (Str.equals_insensitive("ir"))
-  IsIRInstr = true;
+  ProfileKind |= InstrProfKind::IR;
 else if (Str.equals_insensitive("fe"))
-  IsIRInstr = false;
+  ProfileKind |= InstrProfKind::FE;
 else if (Str.equals_insensitive("csir")) {
-  IsIRInstr = true;
-  IsCS = true;
+  ProfileKind |= InstrProfKind::IR;
+  ProfileKind |= InstrProfKind::CS;
 } else if (Str.equals_insensitive("entry_first"))
-  IsEntryFirst = true;
+  ProfileKind |= InstrProfKind::BB;
 else 

[PATCH] D115393: [InstrProf][NFC] Refactor Profile kind into a bitset enum.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish added inline comments.



Comment at: llvm/include/llvm/ProfileData/InstrProf.h:281-282
 
+/// An enum describing the attributes of an instrumented profile.
+enum class InstrProfKind {
+  Unknown = 0x0,

ellis wrote:
> I've been working on a new coverage instrumentation in D116180 that I guess 
> would need to be added this this enum. The plan was to first add function 
> entry coverage, then basic block coverage.
Yes this makes the enum composable and brings it in line with the Variant mask 
encoding in the indexed profile header. Let me go ahead and submit this (after 
rebasing and testing) so that you can rebase your changes. Thanks for the heads 
up!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115393

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

[PATCH] D115393: [InstrProf][NFC] Refactor Profile kind into a bitset enum.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish updated this revision to Diff 398018.
snehasish added a comment.

Rebase.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115393

Files:
  llvm/include/llvm/ProfileData/InstrProf.h
  llvm/include/llvm/ProfileData/InstrProfReader.h
  llvm/include/llvm/ProfileData/InstrProfWriter.h
  llvm/lib/ProfileData/InstrProfReader.cpp
  llvm/lib/ProfileData/InstrProfWriter.cpp
  llvm/tools/llvm-profdata/llvm-profdata.cpp

Index: llvm/tools/llvm-profdata/llvm-profdata.cpp
===
--- llvm/tools/llvm-profdata/llvm-profdata.cpp
+++ llvm/tools/llvm-profdata/llvm-profdata.cpp
@@ -255,9 +255,7 @@
   }
 
   auto Reader = std::move(ReaderOrErr.get());
-  bool IsIRProfile = Reader->isIRLevelProfile();
-  bool HasCSIRProfile = Reader->hasCSIRLevelProfile();
-  if (Error E = WC->Writer.setIsIRLevelProfile(IsIRProfile, HasCSIRProfile)) {
+  if (Error E = WC->Writer.mergeProfileKind(Reader->getProfileKind())) {
 consumeError(std::move(E));
 WC->Errors.emplace_back(
 make_error(
@@ -266,7 +264,6 @@
 Filename);
 return;
   }
-  WC->Writer.setInstrEntryBBEnabled(Reader->instrEntryBBEnabled());
 
   for (auto  : *Reader) {
 if (Remapper)
Index: llvm/lib/ProfileData/InstrProfWriter.cpp
===
--- llvm/lib/ProfileData/InstrProfWriter.cpp
+++ llvm/lib/ProfileData/InstrProfWriter.cpp
@@ -166,9 +166,8 @@
 
 } // end namespace llvm
 
-InstrProfWriter::InstrProfWriter(bool Sparse, bool InstrEntryBBEnabled)
-: Sparse(Sparse), InstrEntryBBEnabled(InstrEntryBBEnabled),
-  InfoObj(new InstrProfRecordWriterTrait()) {}
+InstrProfWriter::InstrProfWriter(bool Sparse)
+: Sparse(Sparse), InfoObj(new InstrProfRecordWriterTrait()) {}
 
 InstrProfWriter::~InstrProfWriter() { delete InfoObj; }
 
@@ -303,13 +302,11 @@
   IndexedInstrProf::Header Header;
   Header.Magic = IndexedInstrProf::Magic;
   Header.Version = IndexedInstrProf::ProfVersion::CurrentVersion;
-  if (ProfileKind == PF_IRLevel)
-Header.Version |= VARIANT_MASK_IR_PROF;
-  if (ProfileKind == PF_IRLevelWithCS) {
+  if (static_cast(ProfileKind & InstrProfKind::IR))
 Header.Version |= VARIANT_MASK_IR_PROF;
+  if (static_cast(ProfileKind & InstrProfKind::CS))
 Header.Version |= VARIANT_MASK_CSIR_PROF;
-  }
-  if (InstrEntryBBEnabled)
+  if (static_cast(ProfileKind & InstrProfKind::BB))
 Header.Version |= VARIANT_MASK_INSTR_ENTRY;
 
   Header.Unused = 0;
@@ -337,7 +334,7 @@
 OS.write(0);
   uint64_t CSSummaryOffset = 0;
   uint64_t CSSummarySize = 0;
-  if (ProfileKind == PF_IRLevelWithCS) {
+  if (static_cast(ProfileKind & InstrProfKind::CS)) {
 CSSummaryOffset = OS.tell();
 CSSummarySize = SummarySize / sizeof(uint64_t);
 for (unsigned I = 0; I < CSSummarySize; I++)
@@ -358,7 +355,7 @@
 
   // For Context Sensitive summary.
   std::unique_ptr TheCSSummary = nullptr;
-  if (ProfileKind == PF_IRLevelWithCS) {
+  if (static_cast(ProfileKind & InstrProfKind::CS)) {
 TheCSSummary = IndexedInstrProf::allocSummary(SummarySize);
 std::unique_ptr CSPS = CSISB.getSummary();
 setSummary(TheCSSummary.get(), *CSPS);
@@ -470,11 +467,13 @@
 }
 
 Error InstrProfWriter::writeText(raw_fd_ostream ) {
-  if (ProfileKind == PF_IRLevel)
-OS << "# IR level Instrumentation Flag\n:ir\n";
-  else if (ProfileKind == PF_IRLevelWithCS)
+  // Check CS first since it implies an IR level profile.
+  if (static_cast(ProfileKind & InstrProfKind::CS))
 OS << "# CSIR level Instrumentation Flag\n:csir\n";
-  if (InstrEntryBBEnabled)
+  else if (static_cast(ProfileKind & InstrProfKind::IR))
+OS << "# IR level Instrumentation Flag\n:ir\n";
+
+  if (static_cast(ProfileKind & InstrProfKind::BB))
 OS << "# Always instrument the function entry block\n:entry_first\n";
   InstrProfSymtab Symtab;
 
Index: llvm/lib/ProfileData/InstrProfReader.cpp
===
--- llvm/lib/ProfileData/InstrProfReader.cpp
+++ llvm/lib/ProfileData/InstrProfReader.cpp
@@ -154,30 +154,24 @@
 // with a leading ':' will be reported an error format.
 Error TextInstrProfReader::readHeader() {
   Symtab.reset(new InstrProfSymtab());
-  bool IsIRInstr = false;
-  bool IsEntryFirst = false;
-  bool IsCS = false;
 
   while (Line->startswith(":")) {
 StringRef Str = Line->substr(1);
 if (Str.equals_insensitive("ir"))
-  IsIRInstr = true;
+  ProfileKind |= InstrProfKind::IR;
 else if (Str.equals_insensitive("fe"))
-  IsIRInstr = false;
+  ProfileKind |= InstrProfKind::FE;
 else if (Str.equals_insensitive("csir")) {
-  IsIRInstr = true;
-  IsCS = true;
+  ProfileKind |= InstrProfKind::IR;
+  ProfileKind |= InstrProfKind::CS;
 } else if (Str.equals_insensitive("entry_first"))
-  IsEntryFirst = true;
+  ProfileKind |= InstrProfKind::BB;
 else 

[PATCH] D115393: [InstrProf][NFC] Refactor Profile kind into a bitset enum.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish added a comment.
Herald added a subscriber: ellis.

ping @xur


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115393

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

[PATCH] D115393: [InstrProf][NFC] Refactor Profile kind into a bitset enum.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish added a comment.

@xur Could you please take a look with a focus on ensuring that this change 
retains the semantics that you expect? Previously there were different enum 
values which indicated CS+IR vs IR only. Now the IR bit is set for both cases 
(to be consistent with the version mask). I believe this was the original 
intent. The only change I had to make was reorder the print in the text format 
writer.




Comment at: llvm/include/llvm/ProfileData/InstrProfWriter.h:48
 public:
-  InstrProfWriter(bool Sparse = false, bool InstrEntryBBEnabled = false);
   ~InstrProfWriter();

tejohnson wrote:
> Was the InstrEntryBBEnabled parameter just never used? I didn't see any 
> changes to callsites.
I didn't find any uses in llvm and I don't see it being used in the original 
commit: https://git.io/JDt8k. There is a chance that it could be used by a 
downstream user but it's unlikely? I'll wait for @xur to chime in.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115393

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

[PATCH] D115393: [InstrProf][NFC] Refactor Profile kind into a bitset enum.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish added inline comments.



Comment at: llvm/include/llvm/ProfileData/InstrProfReader.h:93
 
-  virtual bool isIRLevelProfile() const = 0;
-

tejohnson wrote:
> It seems like these helper methods could still be defined using the new 
> bitset, which would reduce some of the code churn?
My rationale for removing the helpers was 
- to have only one way of checking the bits
- the checks are clustered together so users would get the bitset once rather 
than invoking multiple methods for each check
- extending the bitset would add now mean that adding more helpers?
- retaining the helpers doesn't obviate the need for getProfileKind since the 
simplified code in mergeProfileKind directly operates on the enum value

I agree retaining the helpers reduces code churn and I've made the changes to 
keep them. PTAL, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115393

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

[PATCH] D115393: [InstrProf][NFC] Refactor Profile kind into a bitset enum.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish updated this revision to Diff 392938.
snehasish added a comment.

Remove extra line in PGOInstrumentation.cpp.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115393

Files:
  llvm/include/llvm/ProfileData/InstrProf.h
  llvm/include/llvm/ProfileData/InstrProfReader.h
  llvm/include/llvm/ProfileData/InstrProfWriter.h
  llvm/lib/ProfileData/InstrProfReader.cpp
  llvm/lib/ProfileData/InstrProfWriter.cpp
  llvm/tools/llvm-profdata/llvm-profdata.cpp

Index: llvm/tools/llvm-profdata/llvm-profdata.cpp
===
--- llvm/tools/llvm-profdata/llvm-profdata.cpp
+++ llvm/tools/llvm-profdata/llvm-profdata.cpp
@@ -251,9 +251,7 @@
   }
 
   auto Reader = std::move(ReaderOrErr.get());
-  bool IsIRProfile = Reader->isIRLevelProfile();
-  bool HasCSIRProfile = Reader->hasCSIRLevelProfile();
-  if (Error E = WC->Writer.setIsIRLevelProfile(IsIRProfile, HasCSIRProfile)) {
+  if (Error E = WC->Writer.mergeProfileKind(Reader->getProfileKind())) {
 consumeError(std::move(E));
 WC->Errors.emplace_back(
 make_error(
@@ -262,7 +260,6 @@
 Filename);
 return;
   }
-  WC->Writer.setInstrEntryBBEnabled(Reader->instrEntryBBEnabled());
 
   for (auto  : *Reader) {
 if (Remapper)
Index: llvm/lib/ProfileData/InstrProfWriter.cpp
===
--- llvm/lib/ProfileData/InstrProfWriter.cpp
+++ llvm/lib/ProfileData/InstrProfWriter.cpp
@@ -165,9 +165,8 @@
 
 } // end namespace llvm
 
-InstrProfWriter::InstrProfWriter(bool Sparse, bool InstrEntryBBEnabled)
-: Sparse(Sparse), InstrEntryBBEnabled(InstrEntryBBEnabled),
-  InfoObj(new InstrProfRecordWriterTrait()) {}
+InstrProfWriter::InstrProfWriter(bool Sparse)
+: Sparse(Sparse), InfoObj(new InstrProfRecordWriterTrait()) {}
 
 InstrProfWriter::~InstrProfWriter() { delete InfoObj; }
 
@@ -302,13 +301,11 @@
   IndexedInstrProf::Header Header;
   Header.Magic = IndexedInstrProf::Magic;
   Header.Version = IndexedInstrProf::ProfVersion::CurrentVersion;
-  if (ProfileKind == PF_IRLevel)
-Header.Version |= VARIANT_MASK_IR_PROF;
-  if (ProfileKind == PF_IRLevelWithCS) {
+  if (static_cast(ProfileKind & InstrProfKind::IR))
 Header.Version |= VARIANT_MASK_IR_PROF;
+  if (static_cast(ProfileKind & InstrProfKind::CS))
 Header.Version |= VARIANT_MASK_CSIR_PROF;
-  }
-  if (InstrEntryBBEnabled)
+  if (static_cast(ProfileKind & InstrProfKind::BB))
 Header.Version |= VARIANT_MASK_INSTR_ENTRY;
 
   Header.Unused = 0;
@@ -336,7 +333,7 @@
 OS.write(0);
   uint64_t CSSummaryOffset = 0;
   uint64_t CSSummarySize = 0;
-  if (ProfileKind == PF_IRLevelWithCS) {
+  if (static_cast(ProfileKind & InstrProfKind::CS)) {
 CSSummaryOffset = OS.tell();
 CSSummarySize = SummarySize / sizeof(uint64_t);
 for (unsigned I = 0; I < CSSummarySize; I++)
@@ -357,7 +354,7 @@
 
   // For Context Sensitive summary.
   std::unique_ptr TheCSSummary = nullptr;
-  if (ProfileKind == PF_IRLevelWithCS) {
+  if (static_cast(ProfileKind & InstrProfKind::CS)) {
 TheCSSummary = IndexedInstrProf::allocSummary(SummarySize);
 std::unique_ptr CSPS = CSISB.getSummary();
 setSummary(TheCSSummary.get(), *CSPS);
@@ -469,11 +466,13 @@
 }
 
 Error InstrProfWriter::writeText(raw_fd_ostream ) {
-  if (ProfileKind == PF_IRLevel)
-OS << "# IR level Instrumentation Flag\n:ir\n";
-  else if (ProfileKind == PF_IRLevelWithCS)
+  // Check CS first since it implies an IR level profile.
+  if (static_cast(ProfileKind & InstrProfKind::CS))
 OS << "# CSIR level Instrumentation Flag\n:csir\n";
-  if (InstrEntryBBEnabled)
+  else if (static_cast(ProfileKind & InstrProfKind::IR))
+OS << "# IR level Instrumentation Flag\n:ir\n";
+
+  if (static_cast(ProfileKind & InstrProfKind::BB))
 OS << "# Always instrument the function entry block\n:entry_first\n";
   InstrProfSymtab Symtab;
 
Index: llvm/lib/ProfileData/InstrProfReader.cpp
===
--- llvm/lib/ProfileData/InstrProfReader.cpp
+++ llvm/lib/ProfileData/InstrProfReader.cpp
@@ -151,30 +151,24 @@
 // with a leading ':' will be reported an error format.
 Error TextInstrProfReader::readHeader() {
   Symtab.reset(new InstrProfSymtab());
-  bool IsIRInstr = false;
-  bool IsEntryFirst = false;
-  bool IsCS = false;
 
   while (Line->startswith(":")) {
 StringRef Str = Line->substr(1);
 if (Str.equals_insensitive("ir"))
-  IsIRInstr = true;
+  ProfileKind |= InstrProfKind::IR;
 else if (Str.equals_insensitive("fe"))
-  IsIRInstr = false;
+  ProfileKind |= InstrProfKind::FE;
 else if (Str.equals_insensitive("csir")) {
-  IsIRInstr = true;
-  IsCS = true;
+  ProfileKind |= InstrProfKind::IR;
+  ProfileKind |= InstrProfKind::CS;
 } else if (Str.equals_insensitive("entry_first"))
-  IsEntryFirst = true;
+  

[PATCH] D115393: [InstrProf][NFC] Refactor Profile kind into a bitset enum.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish updated this revision to Diff 392937.
snehasish added a comment.

Reintroduce the helpers while keeping the bitset.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115393

Files:
  llvm/include/llvm/ProfileData/InstrProf.h
  llvm/include/llvm/ProfileData/InstrProfReader.h
  llvm/include/llvm/ProfileData/InstrProfWriter.h
  llvm/lib/ProfileData/InstrProfReader.cpp
  llvm/lib/ProfileData/InstrProfWriter.cpp
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/tools/llvm-profdata/llvm-profdata.cpp

Index: llvm/tools/llvm-profdata/llvm-profdata.cpp
===
--- llvm/tools/llvm-profdata/llvm-profdata.cpp
+++ llvm/tools/llvm-profdata/llvm-profdata.cpp
@@ -251,9 +251,7 @@
   }
 
   auto Reader = std::move(ReaderOrErr.get());
-  bool IsIRProfile = Reader->isIRLevelProfile();
-  bool HasCSIRProfile = Reader->hasCSIRLevelProfile();
-  if (Error E = WC->Writer.setIsIRLevelProfile(IsIRProfile, HasCSIRProfile)) {
+  if (Error E = WC->Writer.mergeProfileKind(Reader->getProfileKind())) {
 consumeError(std::move(E));
 WC->Errors.emplace_back(
 make_error(
@@ -262,7 +260,6 @@
 Filename);
 return;
   }
-  WC->Writer.setInstrEntryBBEnabled(Reader->instrEntryBBEnabled());
 
   for (auto  : *Reader) {
 if (Remapper)
Index: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
===
--- llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
+++ llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
@@ -1827,6 +1827,7 @@
   StringRef("Cannot get PGOReader")));
 return false;
   }
+
   if (!PGOReader->hasCSIRLevelProfile() && IsCS)
 return false;
 
Index: llvm/lib/ProfileData/InstrProfWriter.cpp
===
--- llvm/lib/ProfileData/InstrProfWriter.cpp
+++ llvm/lib/ProfileData/InstrProfWriter.cpp
@@ -165,9 +165,8 @@
 
 } // end namespace llvm
 
-InstrProfWriter::InstrProfWriter(bool Sparse, bool InstrEntryBBEnabled)
-: Sparse(Sparse), InstrEntryBBEnabled(InstrEntryBBEnabled),
-  InfoObj(new InstrProfRecordWriterTrait()) {}
+InstrProfWriter::InstrProfWriter(bool Sparse)
+: Sparse(Sparse), InfoObj(new InstrProfRecordWriterTrait()) {}
 
 InstrProfWriter::~InstrProfWriter() { delete InfoObj; }
 
@@ -302,13 +301,11 @@
   IndexedInstrProf::Header Header;
   Header.Magic = IndexedInstrProf::Magic;
   Header.Version = IndexedInstrProf::ProfVersion::CurrentVersion;
-  if (ProfileKind == PF_IRLevel)
-Header.Version |= VARIANT_MASK_IR_PROF;
-  if (ProfileKind == PF_IRLevelWithCS) {
+  if (static_cast(ProfileKind & InstrProfKind::IR))
 Header.Version |= VARIANT_MASK_IR_PROF;
+  if (static_cast(ProfileKind & InstrProfKind::CS))
 Header.Version |= VARIANT_MASK_CSIR_PROF;
-  }
-  if (InstrEntryBBEnabled)
+  if (static_cast(ProfileKind & InstrProfKind::BB))
 Header.Version |= VARIANT_MASK_INSTR_ENTRY;
 
   Header.Unused = 0;
@@ -336,7 +333,7 @@
 OS.write(0);
   uint64_t CSSummaryOffset = 0;
   uint64_t CSSummarySize = 0;
-  if (ProfileKind == PF_IRLevelWithCS) {
+  if (static_cast(ProfileKind & InstrProfKind::CS)) {
 CSSummaryOffset = OS.tell();
 CSSummarySize = SummarySize / sizeof(uint64_t);
 for (unsigned I = 0; I < CSSummarySize; I++)
@@ -357,7 +354,7 @@
 
   // For Context Sensitive summary.
   std::unique_ptr TheCSSummary = nullptr;
-  if (ProfileKind == PF_IRLevelWithCS) {
+  if (static_cast(ProfileKind & InstrProfKind::CS)) {
 TheCSSummary = IndexedInstrProf::allocSummary(SummarySize);
 std::unique_ptr CSPS = CSISB.getSummary();
 setSummary(TheCSSummary.get(), *CSPS);
@@ -469,11 +466,13 @@
 }
 
 Error InstrProfWriter::writeText(raw_fd_ostream ) {
-  if (ProfileKind == PF_IRLevel)
-OS << "# IR level Instrumentation Flag\n:ir\n";
-  else if (ProfileKind == PF_IRLevelWithCS)
+  // Check CS first since it implies an IR level profile.
+  if (static_cast(ProfileKind & InstrProfKind::CS))
 OS << "# CSIR level Instrumentation Flag\n:csir\n";
-  if (InstrEntryBBEnabled)
+  else if (static_cast(ProfileKind & InstrProfKind::IR))
+OS << "# IR level Instrumentation Flag\n:ir\n";
+
+  if (static_cast(ProfileKind & InstrProfKind::BB))
 OS << "# Always instrument the function entry block\n:entry_first\n";
   InstrProfSymtab Symtab;
 
Index: llvm/lib/ProfileData/InstrProfReader.cpp
===
--- llvm/lib/ProfileData/InstrProfReader.cpp
+++ llvm/lib/ProfileData/InstrProfReader.cpp
@@ -151,30 +151,24 @@
 // with a leading ':' will be reported an error format.
 Error TextInstrProfReader::readHeader() {
   Symtab.reset(new InstrProfSymtab());
-  bool IsIRInstr = false;
-  bool IsEntryFirst = false;
-  bool IsCS = false;
 
   while (Line->startswith(":")) {
 

[PATCH] D115393: [InstrProf][NFC] Refactor Profile kind into a bitset enum.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish updated this revision to Diff 392927.
snehasish added a comment.

Cleanup for review -

- Added some more descriptive comments.
- Removed a couple of unintentional blank lines.
- Removed a couple of commented lines of code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115393

Files:
  clang/lib/Frontend/CompilerInvocation.cpp
  llvm/include/llvm/ProfileData/InstrProf.h
  llvm/include/llvm/ProfileData/InstrProfReader.h
  llvm/include/llvm/ProfileData/InstrProfWriter.h
  llvm/lib/ProfileData/InstrProfReader.cpp
  llvm/lib/ProfileData/InstrProfWriter.cpp
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/tools/llvm-profdata/llvm-profdata.cpp

Index: llvm/tools/llvm-profdata/llvm-profdata.cpp
===
--- llvm/tools/llvm-profdata/llvm-profdata.cpp
+++ llvm/tools/llvm-profdata/llvm-profdata.cpp
@@ -251,9 +251,7 @@
   }
 
   auto Reader = std::move(ReaderOrErr.get());
-  bool IsIRProfile = Reader->isIRLevelProfile();
-  bool HasCSIRProfile = Reader->hasCSIRLevelProfile();
-  if (Error E = WC->Writer.setIsIRLevelProfile(IsIRProfile, HasCSIRProfile)) {
+  if (Error E = WC->Writer.mergeProfileKind(Reader->getProfileKind())) {
 consumeError(std::move(E));
 WC->Errors.emplace_back(
 make_error(
@@ -262,7 +260,6 @@
 Filename);
 return;
   }
-  WC->Writer.setInstrEntryBBEnabled(Reader->instrEntryBBEnabled());
 
   for (auto  : *Reader) {
 if (Remapper)
@@ -2078,7 +2075,8 @@
 exitWithError(std::move(E), Filename);
 
   auto Reader = std::move(ReaderOrErr.get());
-  bool IsIRInstr = Reader->isIRLevelProfile();
+  bool IsIRInstr =
+  static_cast(Reader->getProfileKind() & InstrProfKind::IR);
   size_t ShownFunctions = 0;
   size_t BelowCutoffFunctions = 0;
   int NumVPKind = IPVK_Last - IPVK_First + 1;
@@ -2104,7 +2102,7 @@
 OS << ":ir\n";
 
   for (const auto  : *Reader) {
-if (Reader->isIRLevelProfile()) {
+if (static_cast(Reader->getProfileKind() & InstrProfKind::IR)) {
   bool FuncIsCS = NamedInstrProfRecord::hasCSFlagInHash(Func.Hash);
   if (FuncIsCS != ShowCS)
 continue;
@@ -2203,10 +2201,11 @@
   if (TextFormat)
 return 0;
   std::unique_ptr PS(Builder.getSummary());
-  bool IsIR = Reader->isIRLevelProfile();
+  bool IsIR = static_cast(Reader->getProfileKind() & InstrProfKind::IR);
   OS << "Instrumentation level: " << (IsIR ? "IR" : "Front-end");
   if (IsIR)
-OS << "  entry_first = " << Reader->instrEntryBBEnabled();
+OS << "  entry_first = "
+   << static_cast(Reader->getProfileKind() & InstrProfKind::BB);
   OS << "\n";
   if (ShowAllFunctions || !ShowFunction.empty())
 OS << "Functions shown: " << ShownFunctions << "\n";
Index: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
===
--- llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
+++ llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
@@ -1827,11 +1827,13 @@
   StringRef("Cannot get PGOReader")));
 return false;
   }
-  if (!PGOReader->hasCSIRLevelProfile() && IsCS)
+
+  const InstrProfKind ProfileKind = PGOReader->getProfileKind();
+  if (!static_cast(ProfileKind & InstrProfKind::CS) && IsCS)
 return false;
 
   // TODO: might need to change the warning once the clang option is finalized.
-  if (!PGOReader->isIRLevelProfile()) {
+  if (!static_cast(ProfileKind & InstrProfKind::IR)) {
 Ctx.diagnose(DiagnosticInfoPGOProfile(
 ProfileFileName.data(), "Not an IR level instrumentation profile"));
 return false;
@@ -1852,7 +1854,7 @@
 
   // If the profile marked as always instrument the entry BB, do the
   // same. Note this can be overwritten by the internal option in CFGMST.h
-  bool InstrumentFuncEntry = PGOReader->instrEntryBBEnabled();
+  bool InstrumentFuncEntry = static_cast(ProfileKind & InstrProfKind::BB);
   if (PGOInstrumentEntry.getNumOccurrences() > 0)
 InstrumentFuncEntry = PGOInstrumentEntry;
   for (auto  : M) {
Index: llvm/lib/ProfileData/InstrProfWriter.cpp
===
--- llvm/lib/ProfileData/InstrProfWriter.cpp
+++ llvm/lib/ProfileData/InstrProfWriter.cpp
@@ -165,9 +165,8 @@
 
 } // end namespace llvm
 
-InstrProfWriter::InstrProfWriter(bool Sparse, bool InstrEntryBBEnabled)
-: Sparse(Sparse), InstrEntryBBEnabled(InstrEntryBBEnabled),
-  InfoObj(new InstrProfRecordWriterTrait()) {}
+InstrProfWriter::InstrProfWriter(bool Sparse)
+: Sparse(Sparse), InfoObj(new InstrProfRecordWriterTrait()) {}
 
 InstrProfWriter::~InstrProfWriter() { delete InfoObj; }
 
@@ -302,13 +301,11 @@
   IndexedInstrProf::Header Header;
   Header.Magic = IndexedInstrProf::Magic;
   Header.Version = IndexedInstrProf::ProfVersion::CurrentVersion;
-  if (ProfileKind == PF_IRLevel)
-

[PATCH] D115393: [InstrProf][NFC] Refactor Profile kind into a bitset enum.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish created this revision.
snehasish added reviewers: xur, davidxl.
Herald added subscribers: wenlei, hiraditya.
snehasish requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

This change refactors the ProfileKind enum into a bitset enum to
represent the different attributes a profile can have. This change
simplifies the logic in the instrprof writer when multiple profiles are
merged together. In the future we plan on introducing a new memory
profile section which will extend the enum by one additional entry.
Without this change when accounting for memory profiles will have to be
maintained separately and will make the logic more complex.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D115393

Files:
  clang/lib/Frontend/CompilerInvocation.cpp
  llvm/include/llvm/ProfileData/InstrProf.h
  llvm/include/llvm/ProfileData/InstrProfReader.h
  llvm/include/llvm/ProfileData/InstrProfWriter.h
  llvm/lib/ProfileData/InstrProfReader.cpp
  llvm/lib/ProfileData/InstrProfWriter.cpp
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/tools/llvm-profdata/llvm-profdata.cpp

Index: llvm/tools/llvm-profdata/llvm-profdata.cpp
===
--- llvm/tools/llvm-profdata/llvm-profdata.cpp
+++ llvm/tools/llvm-profdata/llvm-profdata.cpp
@@ -251,9 +251,7 @@
   }
 
   auto Reader = std::move(ReaderOrErr.get());
-  bool IsIRProfile = Reader->isIRLevelProfile();
-  bool HasCSIRProfile = Reader->hasCSIRLevelProfile();
-  if (Error E = WC->Writer.setIsIRLevelProfile(IsIRProfile, HasCSIRProfile)) {
+  if (Error E = WC->Writer.mergeProfileKind(Reader->getProfileKind())) {
 consumeError(std::move(E));
 WC->Errors.emplace_back(
 make_error(
@@ -262,13 +260,13 @@
 Filename);
 return;
   }
-  WC->Writer.setInstrEntryBBEnabled(Reader->instrEntryBBEnabled());
 
   for (auto  : *Reader) {
 if (Remapper)
   I.Name = (*Remapper)(I.Name);
 const StringRef FuncName = I.Name;
 bool Reported = false;
+
 WC->Writer.addRecord(std::move(I), Input.Weight, [&](Error E) {
   if (Reported) {
 consumeError(std::move(E));
@@ -2078,7 +2076,8 @@
 exitWithError(std::move(E), Filename);
 
   auto Reader = std::move(ReaderOrErr.get());
-  bool IsIRInstr = Reader->isIRLevelProfile();
+  bool IsIRInstr =
+  static_cast(Reader->getProfileKind() & InstrProfKind::IR);
   size_t ShownFunctions = 0;
   size_t BelowCutoffFunctions = 0;
   int NumVPKind = IPVK_Last - IPVK_First + 1;
@@ -2104,7 +2103,7 @@
 OS << ":ir\n";
 
   for (const auto  : *Reader) {
-if (Reader->isIRLevelProfile()) {
+if (static_cast(Reader->getProfileKind() & InstrProfKind::IR)) {
   bool FuncIsCS = NamedInstrProfRecord::hasCSFlagInHash(Func.Hash);
   if (FuncIsCS != ShowCS)
 continue;
@@ -2203,10 +2202,11 @@
   if (TextFormat)
 return 0;
   std::unique_ptr PS(Builder.getSummary());
-  bool IsIR = Reader->isIRLevelProfile();
+  bool IsIR = static_cast(Reader->getProfileKind() & InstrProfKind::IR);
   OS << "Instrumentation level: " << (IsIR ? "IR" : "Front-end");
   if (IsIR)
-OS << "  entry_first = " << Reader->instrEntryBBEnabled();
+OS << "  entry_first = "
+   << static_cast(Reader->getProfileKind() & InstrProfKind::BB);
   OS << "\n";
   if (ShowAllFunctions || !ShowFunction.empty())
 OS << "Functions shown: " << ShownFunctions << "\n";
Index: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
===
--- llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
+++ llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
@@ -1827,11 +1827,13 @@
   StringRef("Cannot get PGOReader")));
 return false;
   }
-  if (!PGOReader->hasCSIRLevelProfile() && IsCS)
+
+  const InstrProfKind ProfileKind = PGOReader->getProfileKind();
+  if (!static_cast(ProfileKind & InstrProfKind::CS) && IsCS)
 return false;
 
   // TODO: might need to change the warning once the clang option is finalized.
-  if (!PGOReader->isIRLevelProfile()) {
+  if (!static_cast(ProfileKind & InstrProfKind::IR)) {
 Ctx.diagnose(DiagnosticInfoPGOProfile(
 ProfileFileName.data(), "Not an IR level instrumentation profile"));
 return false;
@@ -1852,7 +1854,7 @@
 
   // If the profile marked as always instrument the entry BB, do the
   // same. Note this can be overwritten by the internal option in CFGMST.h
-  bool InstrumentFuncEntry = PGOReader->instrEntryBBEnabled();
+  bool InstrumentFuncEntry = static_cast(ProfileKind & InstrProfKind::BB);
   if (PGOInstrumentEntry.getNumOccurrences() > 0)
 InstrumentFuncEntry = PGOInstrumentEntry;
   for (auto  : M) {
Index: llvm/lib/ProfileData/InstrProfWriter.cpp
===
--- llvm/lib/ProfileData/InstrProfWriter.cpp
+++ 

[PATCH] D87943: [clang] Remove profile available check for fsplit-machine-functions.

[Snehasish Kumar via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGb86f1af42395: [clang] Remove profile available check for 
fsplit-machine-functions. (authored by snehasish).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87943

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/fsplit-machine-functions.c


Index: clang/test/Driver/fsplit-machine-functions.c
===
--- clang/test/Driver/fsplit-machine-functions.c
+++ clang/test/Driver/fsplit-machine-functions.c
@@ -1,9 +1,8 @@
 // RUN: %clang -### -target x86_64 -fprofile-use=default.profdata 
-fsplit-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
+// RUN: %clang -### -target x86_64 -fsplit-machine-functions %s -c 2>&1 | 
FileCheck -check-prefix=CHECK-OPT %s
 // RUN: %clang -### -target x86_64 -fprofile-use=default.profdata 
-fsplit-machine-functions -fno-split-machine-functions %s -c 2>&1 | FileCheck 
-check-prefix=CHECK-NOOPT %s
-// RUN: %clang -### -target x86_64 -fsplit-machine-functions %s 2>&1 | 
FileCheck -check-prefix=CHECK-WARN %s
 // RUN: not %clang -c -target arm-unknown-linux -fsplit-machine-functions %s 
2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s
 
 // CHECK-OPT:   "-fsplit-machine-functions"
 // CHECK-NOOPT-NOT: "-fsplit-machine-functions"
-// CHECK-WARN:  warning: argument '-fsplit-machine-functions' requires 
profile-guided optimization information
 // CHECK-TRIPLE:error: unsupported option '-fsplit-machine-functions' for 
target
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4916,16 +4916,8 @@
options::OPT_fno_split_machine_functions)) {
 // This codegen pass is only available on x86-elf targets.
 if (Triple.isX86() && Triple.isOSBinFormatELF()) {
-  if (A->getOption().matches(options::OPT_fsplit_machine_functions)) {
-// If the flag is enabled but no profile information is available then
-// emit a warning.
-if (getLastProfileUseArg(Args) || getLastProfileSampleUseArg(Args)) {
-  A->render(Args, CmdArgs);
-} else {
-  D.Diag(diag::warn_drv_diagnostics_hotness_requires_pgo)
-  << A->getAsString(Args);
-}
-  }
+  if (A->getOption().matches(options::OPT_fsplit_machine_functions))
+A->render(Args, CmdArgs);
 } else {
   D.Diag(diag::err_drv_unsupported_opt_for_target)
   << A->getAsString(Args) << TripleStr;


Index: clang/test/Driver/fsplit-machine-functions.c
===
--- clang/test/Driver/fsplit-machine-functions.c
+++ clang/test/Driver/fsplit-machine-functions.c
@@ -1,9 +1,8 @@
 // RUN: %clang -### -target x86_64 -fprofile-use=default.profdata -fsplit-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
+// RUN: %clang -### -target x86_64 -fsplit-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
 // RUN: %clang -### -target x86_64 -fprofile-use=default.profdata -fsplit-machine-functions -fno-split-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-NOOPT %s
-// RUN: %clang -### -target x86_64 -fsplit-machine-functions %s 2>&1 | FileCheck -check-prefix=CHECK-WARN %s
 // RUN: not %clang -c -target arm-unknown-linux -fsplit-machine-functions %s 2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s
 
 // CHECK-OPT:   "-fsplit-machine-functions"
 // CHECK-NOOPT-NOT: "-fsplit-machine-functions"
-// CHECK-WARN:  warning: argument '-fsplit-machine-functions' requires profile-guided optimization information
 // CHECK-TRIPLE:error: unsupported option '-fsplit-machine-functions' for target
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4916,16 +4916,8 @@
options::OPT_fno_split_machine_functions)) {
 // This codegen pass is only available on x86-elf targets.
 if (Triple.isX86() && Triple.isOSBinFormatELF()) {
-  if (A->getOption().matches(options::OPT_fsplit_machine_functions)) {
-// If the flag is enabled but no profile information is available then
-// emit a warning.
-if (getLastProfileUseArg(Args) || getLastProfileSampleUseArg(Args)) {
-  A->render(Args, CmdArgs);
-} else {
-  D.Diag(diag::warn_drv_diagnostics_hotness_requires_pgo)
-  << A->getAsString(Args);
-}
-  }
+  if (A->getOption().matches(options::OPT_fsplit_machine_functions))
+A->render(Args, CmdArgs);
 } else {
   D.Diag(diag::err_drv_unsupported_opt_for_target)
   << 

[PATCH] D87943: [clang] Remove profile availabile check for fsplit-machine-functions.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish marked 2 inline comments as done.
snehasish added a comment.

Thanks for the quick review!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87943

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

[PATCH] D87943: [clang] Remove profile availabile check for fsplit-machine-functions.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish updated this revision to Diff 292899.
snehasish added a comment.

Drop braces, add a test.

- Remove braces from single line if.
- Add a test to make sure that option is passed through even if no profiles 
provided.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87943

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/fsplit-machine-functions.c


Index: clang/test/Driver/fsplit-machine-functions.c
===
--- clang/test/Driver/fsplit-machine-functions.c
+++ clang/test/Driver/fsplit-machine-functions.c
@@ -1,9 +1,8 @@
 // RUN: %clang -### -target x86_64 -fprofile-use=default.profdata 
-fsplit-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
+// RUN: %clang -### -target x86_64 -fsplit-machine-functions %s -c 2>&1 | 
FileCheck -check-prefix=CHECK-OPT %s
 // RUN: %clang -### -target x86_64 -fprofile-use=default.profdata 
-fsplit-machine-functions -fno-split-machine-functions %s -c 2>&1 | FileCheck 
-check-prefix=CHECK-NOOPT %s
-// RUN: %clang -### -target x86_64 -fsplit-machine-functions %s 2>&1 | 
FileCheck -check-prefix=CHECK-WARN %s
 // RUN: not %clang -c -target arm-unknown-linux -fsplit-machine-functions %s 
2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s
 
 // CHECK-OPT:   "-fsplit-machine-functions"
 // CHECK-NOOPT-NOT: "-fsplit-machine-functions"
-// CHECK-WARN:  warning: argument '-fsplit-machine-functions' requires 
profile-guided optimization information
 // CHECK-TRIPLE:error: unsupported option '-fsplit-machine-functions' for 
target
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4916,16 +4916,8 @@
options::OPT_fno_split_machine_functions)) {
 // This codegen pass is only available on x86-elf targets.
 if (Triple.isX86() && Triple.isOSBinFormatELF()) {
-  if (A->getOption().matches(options::OPT_fsplit_machine_functions)) {
-// If the flag is enabled but no profile information is available then
-// emit a warning.
-if (getLastProfileUseArg(Args) || getLastProfileSampleUseArg(Args)) {
-  A->render(Args, CmdArgs);
-} else {
-  D.Diag(diag::warn_drv_diagnostics_hotness_requires_pgo)
-  << A->getAsString(Args);
-}
-  }
+  if (A->getOption().matches(options::OPT_fsplit_machine_functions))
+A->render(Args, CmdArgs);
 } else {
   D.Diag(diag::err_drv_unsupported_opt_for_target)
   << A->getAsString(Args) << TripleStr;


Index: clang/test/Driver/fsplit-machine-functions.c
===
--- clang/test/Driver/fsplit-machine-functions.c
+++ clang/test/Driver/fsplit-machine-functions.c
@@ -1,9 +1,8 @@
 // RUN: %clang -### -target x86_64 -fprofile-use=default.profdata -fsplit-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
+// RUN: %clang -### -target x86_64 -fsplit-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
 // RUN: %clang -### -target x86_64 -fprofile-use=default.profdata -fsplit-machine-functions -fno-split-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-NOOPT %s
-// RUN: %clang -### -target x86_64 -fsplit-machine-functions %s 2>&1 | FileCheck -check-prefix=CHECK-WARN %s
 // RUN: not %clang -c -target arm-unknown-linux -fsplit-machine-functions %s 2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s
 
 // CHECK-OPT:   "-fsplit-machine-functions"
 // CHECK-NOOPT-NOT: "-fsplit-machine-functions"
-// CHECK-WARN:  warning: argument '-fsplit-machine-functions' requires profile-guided optimization information
 // CHECK-TRIPLE:error: unsupported option '-fsplit-machine-functions' for target
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4916,16 +4916,8 @@
options::OPT_fno_split_machine_functions)) {
 // This codegen pass is only available on x86-elf targets.
 if (Triple.isX86() && Triple.isOSBinFormatELF()) {
-  if (A->getOption().matches(options::OPT_fsplit_machine_functions)) {
-// If the flag is enabled but no profile information is available then
-// emit a warning.
-if (getLastProfileUseArg(Args) || getLastProfileSampleUseArg(Args)) {
-  A->render(Args, CmdArgs);
-} else {
-  D.Diag(diag::warn_drv_diagnostics_hotness_requires_pgo)
-  << A->getAsString(Args);
-}
-  }
+  if (A->getOption().matches(options::OPT_fsplit_machine_functions))
+A->render(Args, CmdArgs);
 } else {
   

[PATCH] D87943: [clang] Remove profile availabile check for fsplit-machine-functions.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish created this revision.
snehasish added reviewers: tejohnson, davidxl.
Herald added subscribers: cfe-commits, dexonsmith.
Herald added a project: clang.
snehasish requested review of this revision.

Enforcing a profile available check in the driver does not work with
incremental LTO builds where the LTO backend invocation does not include
the profile flags. At this point the profiles have already been consumed
and the IR contains profile metadata. Instead we always pass through the
-fsplit-machine-functions flag on user request. The pass itself contains
a check to return early if no profile information is available.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D87943

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/fsplit-machine-functions.c


Index: clang/test/Driver/fsplit-machine-functions.c
===
--- clang/test/Driver/fsplit-machine-functions.c
+++ clang/test/Driver/fsplit-machine-functions.c
@@ -1,9 +1,7 @@
 // RUN: %clang -### -target x86_64 -fprofile-use=default.profdata 
-fsplit-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
 // RUN: %clang -### -target x86_64 -fprofile-use=default.profdata 
-fsplit-machine-functions -fno-split-machine-functions %s -c 2>&1 | FileCheck 
-check-prefix=CHECK-NOOPT %s
-// RUN: %clang -### -target x86_64 -fsplit-machine-functions %s 2>&1 | 
FileCheck -check-prefix=CHECK-WARN %s
 // RUN: not %clang -c -target arm-unknown-linux -fsplit-machine-functions %s 
2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s
 
 // CHECK-OPT:   "-fsplit-machine-functions"
 // CHECK-NOOPT-NOT: "-fsplit-machine-functions"
-// CHECK-WARN:  warning: argument '-fsplit-machine-functions' requires 
profile-guided optimization information
 // CHECK-TRIPLE:error: unsupported option '-fsplit-machine-functions' for 
target
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4917,14 +4917,7 @@
 // This codegen pass is only available on x86-elf targets.
 if (Triple.isX86() && Triple.isOSBinFormatELF()) {
   if (A->getOption().matches(options::OPT_fsplit_machine_functions)) {
-// If the flag is enabled but no profile information is available then
-// emit a warning.
-if (getLastProfileUseArg(Args) || getLastProfileSampleUseArg(Args)) {
   A->render(Args, CmdArgs);
-} else {
-  D.Diag(diag::warn_drv_diagnostics_hotness_requires_pgo)
-  << A->getAsString(Args);
-}
   }
 } else {
   D.Diag(diag::err_drv_unsupported_opt_for_target)


Index: clang/test/Driver/fsplit-machine-functions.c
===
--- clang/test/Driver/fsplit-machine-functions.c
+++ clang/test/Driver/fsplit-machine-functions.c
@@ -1,9 +1,7 @@
 // RUN: %clang -### -target x86_64 -fprofile-use=default.profdata -fsplit-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
 // RUN: %clang -### -target x86_64 -fprofile-use=default.profdata -fsplit-machine-functions -fno-split-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-NOOPT %s
-// RUN: %clang -### -target x86_64 -fsplit-machine-functions %s 2>&1 | FileCheck -check-prefix=CHECK-WARN %s
 // RUN: not %clang -c -target arm-unknown-linux -fsplit-machine-functions %s 2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s
 
 // CHECK-OPT:   "-fsplit-machine-functions"
 // CHECK-NOOPT-NOT: "-fsplit-machine-functions"
-// CHECK-WARN:  warning: argument '-fsplit-machine-functions' requires profile-guided optimization information
 // CHECK-TRIPLE:error: unsupported option '-fsplit-machine-functions' for target
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4917,14 +4917,7 @@
 // This codegen pass is only available on x86-elf targets.
 if (Triple.isX86() && Triple.isOSBinFormatELF()) {
   if (A->getOption().matches(options::OPT_fsplit_machine_functions)) {
-// If the flag is enabled but no profile information is available then
-// emit a warning.
-if (getLastProfileUseArg(Args) || getLastProfileSampleUseArg(Args)) {
   A->render(Args, CmdArgs);
-} else {
-  D.Diag(diag::warn_drv_diagnostics_hotness_requires_pgo)
-  << A->getAsString(Args);
-}
   }
 } else {
   D.Diag(diag::err_drv_unsupported_opt_for_target)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87047: [clang] Add command line options for the Machine Function Splitter.

[Snehasish Kumar via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGf1a3ab904439: [clang] Add a command line flag for the 
Machine Function Splitter. (authored by snehasish).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87047

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Driver/fsplit-machine-functions.c


Index: clang/test/Driver/fsplit-machine-functions.c
===
--- /dev/null
+++ clang/test/Driver/fsplit-machine-functions.c
@@ -0,0 +1,9 @@
+// RUN: %clang -### -target x86_64 -fprofile-use=default.profdata 
-fsplit-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
+// RUN: %clang -### -target x86_64 -fprofile-use=default.profdata 
-fsplit-machine-functions -fno-split-machine-functions %s -c 2>&1 | FileCheck 
-check-prefix=CHECK-NOOPT %s
+// RUN: %clang -### -target x86_64 -fsplit-machine-functions %s 2>&1 | 
FileCheck -check-prefix=CHECK-WARN %s
+// RUN: not %clang -c -target arm-unknown-linux -fsplit-machine-functions %s 
2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s
+
+// CHECK-OPT:   "-fsplit-machine-functions"
+// CHECK-NOOPT-NOT: "-fsplit-machine-functions"
+// CHECK-WARN:  warning: argument '-fsplit-machine-functions' requires 
profile-guided optimization information
+// CHECK-TRIPLE:error: unsupported option '-fsplit-machine-functions' for 
target
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -998,6 +998,8 @@
   Opts.UniqueInternalLinkageNames =
   Args.hasArg(OPT_funique_internal_linkage_names);
 
+  Opts.SplitMachineFunctions = Args.hasArg(OPT_fsplit_machine_functions);
+
   Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
 
   Opts.NoUseJumpTables = Args.hasArg(OPT_fno_jump_tables);
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4911,6 +4911,26 @@
options::OPT_fno_unique_basic_block_section_names, false))
 CmdArgs.push_back("-funique-basic-block-section-names");
 
+  if (Arg *A = Args.getLastArg(options::OPT_fsplit_machine_functions,
+   options::OPT_fno_split_machine_functions)) {
+// This codegen pass is only available on x86-elf targets.
+if (Triple.isX86() && Triple.isOSBinFormatELF()) {
+  if (A->getOption().matches(options::OPT_fsplit_machine_functions)) {
+// If the flag is enabled but no profile information is available then
+// emit a warning.
+if (getLastProfileUseArg(Args) || getLastProfileSampleUseArg(Args)) {
+  A->render(Args, CmdArgs);
+} else {
+  D.Diag(diag::warn_drv_diagnostics_hotness_requires_pgo)
+  << A->getAsString(Args);
+}
+  }
+} else {
+  D.Diag(diag::err_drv_unsupported_opt_for_target)
+  << A->getAsString(Args) << TripleStr;
+}
+  }
+
   Args.AddLastArg(CmdArgs, options::OPT_finstrument_functions,
   options::OPT_finstrument_functions_after_inlining,
   options::OPT_finstrument_function_entry_bare);
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -514,6 +514,7 @@
   Options.BBSectionsFuncListBuf = std::move(*MBOrErr);
   }
 
+  Options.EnableMachineFunctionSplitter = CodeGenOpts.SplitMachineFunctions;
   Options.FunctionSections = CodeGenOpts.FunctionSections;
   Options.DataSections = CodeGenOpts.DataSections;
   Options.UniqueSectionNames = CodeGenOpts.UniqueSectionNames;
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1996,6 +1996,9 @@
 defm unique_section_names : OptOutFFlag<"unique-section-names",
   "", "Don't use unique names for text and data sections">;
 
+defm split_machine_functions: OptInFFlag<"split-machine-functions",
+  "Enable", "Disable", " late function splitting using profile information 
(x86 ELF)">;
+
 defm strict_return : OptOutFFlag<"strict-return", "",
   "Don't treat control flow paths that fall off the end of a non-void function 
as unreachable">;
 
Index: clang/include/clang/Basic/CodeGenOptions.def
===
--- clang/include/clang/Basic/CodeGenOptions.def
+++ 

[PATCH] D87047: [clang] Add command line options for the Machine Function Splitter.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish added a comment.

Thanks for the review.




Comment at: clang/include/clang/Driver/Options.td:2000
+defm split_machine_functions: OptInFFlag<"split-machine-functions",
+  "Enable", "Disable", " late function splitting using profile information 
(x86-elf only)">;
+

MaskRay wrote:
> MaskRay wrote:
> > If this can be adapted to other targets, there is no need to specifically 
> > mention x86-elf only ("x86 ELF")
> So I take quickly a look at rG94faadaca4e1704f674d2e9d4a1d25643b9ca52c. There 
> is nothing ELF specific, right? At least I don't expect AArch64 ELF to fail. 
> If you do think this merits a "ELF only" comment, adding it seems feasible.
The machine function splitter uses the basic block sections feature. This has 
not been tested on AArch64, there is some target specific code in the CFI 
handling as well idiosyncrasies of each target which need to be worked out. I'm 
going to leave this as it is since the checks included in this patch enforce 
x86 & ELF. This is on our longer term roadmap and we will revisit this in the 
future.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87047

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

[PATCH] D87047: [clang] Add command line options for the Machine Function Splitter.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish updated this revision to Diff 291986.
snehasish marked 2 inline comments as done.
snehasish added a comment.

Update the test.

- Added "warning: " prefix to be checked.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87047

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Driver/fsplit-machine-functions.c


Index: clang/test/Driver/fsplit-machine-functions.c
===
--- /dev/null
+++ clang/test/Driver/fsplit-machine-functions.c
@@ -0,0 +1,9 @@
+// RUN: %clang -### -target x86_64 -fprofile-use=default.profdata 
-fsplit-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
+// RUN: %clang -### -target x86_64 -fprofile-use=default.profdata 
-fsplit-machine-functions -fno-split-machine-functions %s -c 2>&1 | FileCheck 
-check-prefix=CHECK-NOOPT %s
+// RUN: %clang -### -target x86_64 -fsplit-machine-functions %s 2>&1 | 
FileCheck -check-prefix=CHECK-WARN %s
+// RUN: not %clang -c -target arm-unknown-linux -fsplit-machine-functions %s 
2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s
+
+// CHECK-OPT:   "-fsplit-machine-functions"
+// CHECK-NOOPT-NOT: "-fsplit-machine-functions"
+// CHECK-WARN:  warning: argument '-fsplit-machine-functions' requires 
profile-guided optimization information
+// CHECK-TRIPLE:error: unsupported option '-fsplit-machine-functions' for 
target
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -998,6 +998,8 @@
   Opts.UniqueInternalLinkageNames =
   Args.hasArg(OPT_funique_internal_linkage_names);
 
+  Opts.SplitMachineFunctions = Args.hasArg(OPT_fsplit_machine_functions);
+
   Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
 
   Opts.NoUseJumpTables = Args.hasArg(OPT_fno_jump_tables);
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4911,6 +4911,26 @@
options::OPT_fno_unique_basic_block_section_names, false))
 CmdArgs.push_back("-funique-basic-block-section-names");
 
+  if (Arg *A = Args.getLastArg(options::OPT_fsplit_machine_functions,
+   options::OPT_fno_split_machine_functions)) {
+// This codegen pass is only available on x86-elf targets.
+if (Triple.isX86() && Triple.isOSBinFormatELF()) {
+  if (A->getOption().matches(options::OPT_fsplit_machine_functions)) {
+// If the flag is enabled but no profile information is available then
+// emit a warning.
+if (getLastProfileUseArg(Args) || getLastProfileSampleUseArg(Args)) {
+  A->render(Args, CmdArgs);
+} else {
+  D.Diag(diag::warn_drv_diagnostics_hotness_requires_pgo)
+  << A->getAsString(Args);
+}
+  }
+} else {
+  D.Diag(diag::err_drv_unsupported_opt_for_target)
+  << A->getAsString(Args) << TripleStr;
+}
+  }
+
   Args.AddLastArg(CmdArgs, options::OPT_finstrument_functions,
   options::OPT_finstrument_functions_after_inlining,
   options::OPT_finstrument_function_entry_bare);
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -514,6 +514,7 @@
   Options.BBSectionsFuncListBuf = std::move(*MBOrErr);
   }
 
+  Options.EnableMachineFunctionSplitter = CodeGenOpts.SplitMachineFunctions;
   Options.FunctionSections = CodeGenOpts.FunctionSections;
   Options.DataSections = CodeGenOpts.DataSections;
   Options.UniqueSectionNames = CodeGenOpts.UniqueSectionNames;
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1996,6 +1996,9 @@
 defm unique_section_names : OptOutFFlag<"unique-section-names",
   "", "Don't use unique names for text and data sections">;
 
+defm split_machine_functions: OptInFFlag<"split-machine-functions",
+  "Enable", "Disable", " late function splitting using profile information 
(x86 ELF)">;
+
 defm strict_return : OptOutFFlag<"strict-return", "",
   "Don't treat control flow paths that fall off the end of a non-void function 
as unreachable">;
 
Index: clang/include/clang/Basic/CodeGenOptions.def
===
--- clang/include/clang/Basic/CodeGenOptions.def
+++ clang/include/clang/Basic/CodeGenOptions.def
@@ -162,6 

[PATCH] D87047: [clang] Add command line options for the Machine Function Splitter.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish updated this revision to Diff 291775.
snehasish added a comment.

Remove unnecessary includes, update doc text.

- Update doctext.
- Remove no longer necessary includes to frontend driver diagnostics.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87047

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Driver/fsplit-machine-functions.c


Index: clang/test/Driver/fsplit-machine-functions.c
===
--- /dev/null
+++ clang/test/Driver/fsplit-machine-functions.c
@@ -0,0 +1,9 @@
+// RUN: %clang -### -target x86_64 -fprofile-use=default.profdata 
-fsplit-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
+// RUN: %clang -### -target x86_64 -fprofile-use=default.profdata 
-fsplit-machine-functions -fno-split-machine-functions %s -c 2>&1 | FileCheck 
-check-prefix=CHECK-NOOPT %s
+// RUN: %clang -### -target x86_64 -fsplit-machine-functions %s 2>&1 | 
FileCheck -check-prefix=CHECK-WARN %s
+// RUN: not %clang -c -target arm-unknown-linux -fsplit-machine-functions %s 
2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s
+
+// CHECK-OPT:   "-fsplit-machine-functions"
+// CHECK-NOOPT-NOT: "-fsplit-machine-functions"
+// CHECK-WARN:  argument '-fsplit-machine-functions' requires 
profile-guided optimization information
+// CHECK-TRIPLE:error: unsupported option '-fsplit-machine-functions' for 
target
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -998,6 +998,8 @@
   Opts.UniqueInternalLinkageNames =
   Args.hasArg(OPT_funique_internal_linkage_names);
 
+  Opts.SplitMachineFunctions = Args.hasArg(OPT_fsplit_machine_functions);
+
   Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
 
   Opts.NoUseJumpTables = Args.hasArg(OPT_fno_jump_tables);
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4911,6 +4911,26 @@
options::OPT_fno_unique_basic_block_section_names, false))
 CmdArgs.push_back("-funique-basic-block-section-names");
 
+  if (Arg *A = Args.getLastArg(options::OPT_fsplit_machine_functions,
+   options::OPT_fno_split_machine_functions)) {
+// This codegen pass is only available on x86-elf targets.
+if (Triple.isX86() && Triple.isOSBinFormatELF()) {
+  if (A->getOption().matches(options::OPT_fsplit_machine_functions)) {
+// If the flag is enabled but no profile information is available then
+// emit a warning.
+if (getLastProfileUseArg(Args) || getLastProfileSampleUseArg(Args)) {
+  A->render(Args, CmdArgs);
+} else {
+  D.Diag(diag::warn_drv_diagnostics_hotness_requires_pgo)
+  << A->getAsString(Args);
+}
+  }
+} else {
+  D.Diag(diag::err_drv_unsupported_opt_for_target)
+  << A->getAsString(Args) << TripleStr;
+}
+  }
+
   Args.AddLastArg(CmdArgs, options::OPT_finstrument_functions,
   options::OPT_finstrument_functions_after_inlining,
   options::OPT_finstrument_function_entry_bare);
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -514,6 +514,7 @@
   Options.BBSectionsFuncListBuf = std::move(*MBOrErr);
   }
 
+  Options.EnableMachineFunctionSplitter = CodeGenOpts.SplitMachineFunctions;
   Options.FunctionSections = CodeGenOpts.FunctionSections;
   Options.DataSections = CodeGenOpts.DataSections;
   Options.UniqueSectionNames = CodeGenOpts.UniqueSectionNames;
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1996,6 +1996,9 @@
 defm unique_section_names : OptOutFFlag<"unique-section-names",
   "", "Don't use unique names for text and data sections">;
 
+defm split_machine_functions: OptInFFlag<"split-machine-functions",
+  "Enable", "Disable", " late function splitting using profile information 
(x86 ELF)">;
+
 defm strict_return : OptOutFFlag<"strict-return", "",
   "Don't treat control flow paths that fall off the end of a non-void function 
as unreachable">;
 
Index: clang/include/clang/Basic/CodeGenOptions.def
===
--- clang/include/clang/Basic/CodeGenOptions.def
+++ 

[PATCH] D87047: [clang] Add command line options for the Machine Function Splitter.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish marked 3 inline comments as done.
snehasish added a comment.

That makes sense. I moved the check to lib/Driver/ToolChains/Clang.cpp and 
updated the test. Seems cleaner to have all the checks in one place.
PTAL, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87047

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

[PATCH] D87047: [clang] Add command line options for the Machine Function Splitter.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish updated this revision to Diff 291771.
snehasish marked an inline comment as done.
snehasish added a comment.

Check profile flag in Driver, update test.

- Move the profile missing warning check to the Driver.
- Update the test to use -### for CHECK-WARN.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87047

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Driver/fsplit-machine-functions.c

Index: clang/test/Driver/fsplit-machine-functions.c
===
--- /dev/null
+++ clang/test/Driver/fsplit-machine-functions.c
@@ -0,0 +1,9 @@
+// RUN: %clang -### -target x86_64 -fprofile-use=default.profdata -fsplit-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
+// RUN: %clang -### -target x86_64 -fprofile-use=default.profdata -fsplit-machine-functions -fno-split-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-NOOPT %s
+// RUN: %clang -### -target x86_64 -fsplit-machine-functions %s 2>&1 | FileCheck -check-prefix=CHECK-WARN %s
+// RUN: not %clang -c -target arm-unknown-linux -fsplit-machine-functions %s 2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s
+
+// CHECK-OPT:   "-fsplit-machine-functions"
+// CHECK-NOOPT-NOT: "-fsplit-machine-functions"
+// CHECK-WARN:  argument '-fsplit-machine-functions' requires profile-guided optimization information
+// CHECK-TRIPLE:error: unsupported option '-fsplit-machine-functions' for target
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -998,6 +998,8 @@
   Opts.UniqueInternalLinkageNames =
   Args.hasArg(OPT_funique_internal_linkage_names);
 
+  Opts.SplitMachineFunctions = Args.hasArg(OPT_fsplit_machine_functions);
+
   Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
 
   Opts.NoUseJumpTables = Args.hasArg(OPT_fno_jump_tables);
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -24,6 +24,7 @@
 #include "PS4CPU.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/CodeGenOptions.h"
+#include "clang/Basic/DiagnosticFrontend.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/ObjCRuntime.h"
 #include "clang/Basic/Version.h"
@@ -4911,6 +4912,26 @@
options::OPT_fno_unique_basic_block_section_names, false))
 CmdArgs.push_back("-funique-basic-block-section-names");
 
+  if (Arg *A = Args.getLastArg(options::OPT_fsplit_machine_functions,
+   options::OPT_fno_split_machine_functions)) {
+// This codegen pass is only available on x86-elf targets.
+if (Triple.isX86() && Triple.isOSBinFormatELF()) {
+  if (A->getOption().matches(options::OPT_fsplit_machine_functions)) {
+// If the flag is enabled but no profile information is available then
+// emit a warning.
+if (getLastProfileUseArg(Args) || getLastProfileSampleUseArg(Args)) {
+  A->render(Args, CmdArgs);
+} else {
+  D.Diag(diag::warn_drv_diagnostics_hotness_requires_pgo)
+  << A->getAsString(Args);
+}
+  }
+} else {
+  D.Diag(diag::err_drv_unsupported_opt_for_target)
+  << A->getAsString(Args) << TripleStr;
+}
+  }
+
   Args.AddLastArg(CmdArgs, options::OPT_finstrument_functions,
   options::OPT_finstrument_functions_after_inlining,
   options::OPT_finstrument_function_entry_bare);
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -9,6 +9,7 @@
 #include "clang/CodeGen/BackendUtil.h"
 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/DiagnosticFrontend.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/TargetOptions.h"
 #include "clang/Frontend/FrontendDiagnostic.h"
@@ -514,6 +515,7 @@
   Options.BBSectionsFuncListBuf = std::move(*MBOrErr);
   }
 
+  Options.EnableMachineFunctionSplitter = CodeGenOpts.SplitMachineFunctions;
   Options.FunctionSections = CodeGenOpts.FunctionSections;
   Options.DataSections = CodeGenOpts.DataSections;
   Options.UniqueSectionNames = CodeGenOpts.UniqueSectionNames;
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1996,6 +1996,9 @@
 defm unique_section_names : 

[PATCH] D87047: [clang] Add command line options for the Machine Function Splitter.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish marked an inline comment as done.
snehasish added inline comments.



Comment at: clang/test/Driver/fsplit-machine-functions.c:3
+// RUN: %clang -### -target x86_64 -fprofile-use=default.profdata 
-fsplit-machine-functions -fno-split-machine-functions %s -c 2>&1 | FileCheck 
-check-prefix=CHECK-NOOPT %s
+// RUN: %clang -c -target x86_64 -fsplit-machine-functions %s -c 2>&1 | 
FileCheck -check-prefix=CHECK-WARN %s
+// RUN: not %clang -c -target arm-unknown-linux -fsplit-machine-functions %s 
2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s

MaskRay wrote:
> There are two -c on this line.
> 
> Testing warnings can use `-###` as well.
Removed the extra -c. 
The warning is emitted when CreateTargetMachine (and its children) are called. 
So using `-###` instead of `-c` for the CHECK-WARN test is not sufficient as 
far as I can tell.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87047

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

[PATCH] D87047: [clang] Add command line options for the Machine Function Splitter.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish updated this revision to Diff 291758.
snehasish added a comment.

Remove extra -c from test command line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87047

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Driver/fsplit-machine-functions.c

Index: clang/test/Driver/fsplit-machine-functions.c
===
--- /dev/null
+++ clang/test/Driver/fsplit-machine-functions.c
@@ -0,0 +1,9 @@
+// RUN: %clang -### -target x86_64 -fprofile-use=default.profdata -fsplit-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
+// RUN: %clang -### -target x86_64 -fprofile-use=default.profdata -fsplit-machine-functions -fno-split-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-NOOPT %s
+// RUN: %clang -c -target x86_64 -fsplit-machine-functions %s 2>&1 | FileCheck -check-prefix=CHECK-WARN %s
+// RUN: not %clang -c -target arm-unknown-linux -fsplit-machine-functions %s 2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s
+
+// CHECK-OPT:   "-fsplit-machine-functions"
+// CHECK-NOOPT-NOT: "-fsplit-machine-functions"
+// CHECK-WARN:  ignored -fsplit-machine-functions, no profile provided via -fprofile-use
+// CHECK-TRIPLE:error: unsupported option '-fsplit-machine-functions' for target
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -998,6 +998,8 @@
   Opts.UniqueInternalLinkageNames =
   Args.hasArg(OPT_funique_internal_linkage_names);
 
+  Opts.SplitMachineFunctions = Args.hasArg(OPT_fsplit_machine_functions);
+
   Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
 
   Opts.NoUseJumpTables = Args.hasArg(OPT_fno_jump_tables);
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4911,6 +4911,17 @@
options::OPT_fno_unique_basic_block_section_names, false))
 CmdArgs.push_back("-funique-basic-block-section-names");
 
+  if (Arg *A = Args.getLastArg(options::OPT_fsplit_machine_functions,
+   options::OPT_fno_split_machine_functions)) {
+if (Triple.isX86() && Triple.isOSBinFormatELF()) {
+  if (A->getOption().matches(options::OPT_fsplit_machine_functions))
+A->render(Args, CmdArgs);
+} else {
+  D.Diag(diag::err_drv_unsupported_opt_for_target)
+  << A->getAsString(Args) << TripleStr;
+}
+  }
+
   Args.AddLastArg(CmdArgs, options::OPT_finstrument_functions,
   options::OPT_finstrument_functions_after_inlining,
   options::OPT_finstrument_function_entry_bare);
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -9,6 +9,7 @@
 #include "clang/CodeGen/BackendUtil.h"
 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/DiagnosticFrontend.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/TargetOptions.h"
 #include "clang/Frontend/FrontendDiagnostic.h"
@@ -514,6 +515,15 @@
   Options.BBSectionsFuncListBuf = std::move(*MBOrErr);
   }
 
+  if (CodeGenOpts.SplitMachineFunctions) {
+if (CodeGenOpts.getProfileUse() != CodeGenOptions::ProfileNone)
+  Options.EnableMachineFunctionSplitter = true;
+else
+  Diags.Report(diag::warn_fe_backend_optimization_failure)
+  << "ignored -fsplit-machine-functions, no profile provided via "
+ "-fprofile-use";
+  }
+
   Options.FunctionSections = CodeGenOpts.FunctionSections;
   Options.DataSections = CodeGenOpts.DataSections;
   Options.UniqueSectionNames = CodeGenOpts.UniqueSectionNames;
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1996,6 +1996,9 @@
 defm unique_section_names : OptOutFFlag<"unique-section-names",
   "", "Don't use unique names for text and data sections">;
 
+defm split_machine_functions: OptInFFlag<"split-machine-functions",
+  "Enable", "Disable", " late function splitting using profile information (x86-elf only)">;
+
 defm strict_return : OptOutFFlag<"strict-return", "",
   "Don't treat control flow paths that fall off the end of a non-void function as unreachable">;
 
Index: clang/include/clang/Basic/CodeGenOptions.def
===
--- 

[PATCH] D87047: [clang] Add command line options for the Machine Function Splitter.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish added a comment.

@MaskRay ping, let me know if you have any further comments. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87047

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

[PATCH] D87047: [clang] Add command line options for the Machine Function Splitter.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish updated this revision to Diff 291728.
snehasish added a comment.

Rebased patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87047

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Driver/fsplit-machine-functions.c

Index: clang/test/Driver/fsplit-machine-functions.c
===
--- /dev/null
+++ clang/test/Driver/fsplit-machine-functions.c
@@ -0,0 +1,9 @@
+// RUN: %clang -### -target x86_64 -fprofile-use=default.profdata -fsplit-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
+// RUN: %clang -### -target x86_64 -fprofile-use=default.profdata -fsplit-machine-functions -fno-split-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-NOOPT %s
+// RUN: %clang -c -target x86_64 -fsplit-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-WARN %s
+// RUN: not %clang -c -target arm-unknown-linux -fsplit-machine-functions %s 2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s
+
+// CHECK-OPT:   "-fsplit-machine-functions"
+// CHECK-NOOPT-NOT: "-fsplit-machine-functions"
+// CHECK-WARN:  ignored -fsplit-machine-functions, no profile provided via -fprofile-use
+// CHECK-TRIPLE:error: unsupported option '-fsplit-machine-functions' for target
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -998,6 +998,8 @@
   Opts.UniqueInternalLinkageNames =
   Args.hasArg(OPT_funique_internal_linkage_names);
 
+  Opts.SplitMachineFunctions = Args.hasArg(OPT_fsplit_machine_functions);
+
   Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
 
   Opts.NoUseJumpTables = Args.hasArg(OPT_fno_jump_tables);
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4911,6 +4911,17 @@
options::OPT_fno_unique_basic_block_section_names, false))
 CmdArgs.push_back("-funique-basic-block-section-names");
 
+  if (Arg *A = Args.getLastArg(options::OPT_fsplit_machine_functions,
+   options::OPT_fno_split_machine_functions)) {
+if (Triple.isX86() && Triple.isOSBinFormatELF()) {
+  if (A->getOption().matches(options::OPT_fsplit_machine_functions))
+A->render(Args, CmdArgs);
+} else {
+  D.Diag(diag::err_drv_unsupported_opt_for_target)
+  << A->getAsString(Args) << TripleStr;
+}
+  }
+
   Args.AddLastArg(CmdArgs, options::OPT_finstrument_functions,
   options::OPT_finstrument_functions_after_inlining,
   options::OPT_finstrument_function_entry_bare);
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -9,6 +9,7 @@
 #include "clang/CodeGen/BackendUtil.h"
 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/DiagnosticFrontend.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/TargetOptions.h"
 #include "clang/Frontend/FrontendDiagnostic.h"
@@ -514,6 +515,15 @@
   Options.BBSectionsFuncListBuf = std::move(*MBOrErr);
   }
 
+  if (CodeGenOpts.SplitMachineFunctions) {
+if (CodeGenOpts.getProfileUse() != CodeGenOptions::ProfileNone)
+  Options.EnableMachineFunctionSplitter = true;
+else
+  Diags.Report(diag::warn_fe_backend_optimization_failure)
+  << "ignored -fsplit-machine-functions, no profile provided via "
+ "-fprofile-use";
+  }
+
   Options.FunctionSections = CodeGenOpts.FunctionSections;
   Options.DataSections = CodeGenOpts.DataSections;
   Options.UniqueSectionNames = CodeGenOpts.UniqueSectionNames;
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1996,6 +1996,9 @@
 defm unique_section_names : OptOutFFlag<"unique-section-names",
   "", "Don't use unique names for text and data sections">;
 
+defm split_machine_functions: OptInFFlag<"split-machine-functions",
+  "Enable", "Disable", " late function splitting using profile information (x86-elf only)">;
+
 defm strict_return : OptOutFFlag<"strict-return", "",
   "Don't treat control flow paths that fall off the end of a non-void function as unreachable">;
 
Index: clang/include/clang/Basic/CodeGenOptions.def
===
--- 

[PATCH] D87047: [clang] Add command line options for the Machine Function Splitter.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish marked 3 inline comments as done.
snehasish added a subscriber: dblaikie.
snehasish added a comment.

> It feels wrong that the assembly+llvm-profdata test is in clang/test

I agree with @dblaikie and your assessment that it feels wrong to add such a 
test to clang. In the first version of this patch, the test served the purpose 
of representing the canonical usage with profdata as well as check for the 
plumbing of the profile along with the presence of the flag. Enhancing the 
driver test removed the need for the former while adding a diagnostic warning 
message (and a check for it) removes the need for the latter. I've removed the 
clang codegen test.

Thanks for the detailed review, PTAL.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87047

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

[PATCH] D87047: [clang] Add command line options for the Machine Function Splitter.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish updated this revision to Diff 291292.
snehasish added a comment.

Remove clang/CodeGen test, update arg render logic.

- Removed the clang/CodeGen test.
- Added a check for the option to be rendered.
- Fixed extra flags in driver test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87047

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Driver/fsplit-machine-functions.c

Index: clang/test/Driver/fsplit-machine-functions.c
===
--- /dev/null
+++ clang/test/Driver/fsplit-machine-functions.c
@@ -0,0 +1,9 @@
+// RUN: %clang -### -target x86_64 -fprofile-use=default.profdata -fsplit-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
+// RUN: %clang -### -target x86_64 -fprofile-use=default.profdata -fsplit-machine-functions -fno-split-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-NOOPT %s
+// RUN: %clang -c -target x86_64 -fsplit-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-WARN %s
+// RUN: not %clang -c -target arm-unknown-linux -fsplit-machine-functions %s 2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s
+
+// CHECK-OPT:   "-fsplit-machine-functions"
+// CHECK-NOOPT-NOT: "-fsplit-machine-functions"
+// CHECK-WARN:  ignored -fsplit-machine-functions, no profile provided via -fprofile-use
+// CHECK-TRIPLE:error: unsupported option '-fsplit-machine-functions' for target
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -998,6 +998,8 @@
   Opts.UniqueInternalLinkageNames =
   Args.hasArg(OPT_funique_internal_linkage_names);
 
+  Opts.SplitMachineFunctions = Args.hasArg(OPT_fsplit_machine_functions);
+
   Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
 
   Opts.NoUseJumpTables = Args.hasArg(OPT_fno_jump_tables);
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4911,6 +4911,17 @@
options::OPT_fno_unique_basic_block_section_names, false))
 CmdArgs.push_back("-funique-basic-block-section-names");
 
+  if (Arg *A = Args.getLastArg(options::OPT_fsplit_machine_functions,
+   options::OPT_fno_split_machine_functions)) {
+if (Triple.isX86() && Triple.isOSBinFormatELF()) {
+  if (A->getOption().matches(options::OPT_fsplit_machine_functions))
+A->render(Args, CmdArgs);
+} else {
+  D.Diag(diag::err_drv_unsupported_opt_for_target)
+  << A->getAsString(Args) << TripleStr;
+}
+  }
+
   Args.AddLastArg(CmdArgs, options::OPT_finstrument_functions,
   options::OPT_finstrument_functions_after_inlining,
   options::OPT_finstrument_function_entry_bare);
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -9,6 +9,7 @@
 #include "clang/CodeGen/BackendUtil.h"
 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/DiagnosticFrontend.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/TargetOptions.h"
 #include "clang/Frontend/FrontendDiagnostic.h"
@@ -514,6 +515,15 @@
   Options.BBSectionsFuncListBuf = std::move(*MBOrErr);
   }
 
+  if (CodeGenOpts.SplitMachineFunctions) {
+if (CodeGenOpts.getProfileUse() != CodeGenOptions::ProfileNone)
+  Options.EnableMachineFunctionSplitter = true;
+else
+  Diags.Report(diag::warn_fe_backend_optimization_failure)
+  << "ignored -fsplit-machine-functions, no profile provided via "
+ "-fprofile-use";
+  }
+
   Options.FunctionSections = CodeGenOpts.FunctionSections;
   Options.DataSections = CodeGenOpts.DataSections;
   Options.UniqueSectionNames = CodeGenOpts.UniqueSectionNames;
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1989,6 +1989,9 @@
 defm unique_section_names : OptOutFFlag<"unique-section-names",
   "", "Don't use unique names for text and data sections">;
 
+defm split_machine_functions: OptInFFlag<"split-machine-functions",
+  "Enable", "Disable", " late function splitting using profile information (x86-elf only)">;
+
 defm strict_return : OptOutFFlag<"strict-return", "",
   "Don't treat control flow paths that fall off the end of a non-void function as unreachable">;
 
Index: 

[PATCH] D87047: [clang] Add command line options for the Machine Function Splitter.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish marked an inline comment as done.
snehasish added inline comments.



Comment at: clang/test/CodeGen/split-machine-functions.c:3
+
+// RUN: echo "foo"> %t.proftext
+// RUN: echo "# Func Hash:"   >> %t.proftext

snehasish wrote:
> MaskRay wrote:
> > Consider `RUN: split-file`
> > 
> > Search for this string for some examples.
> Looks much cleaner with split-file overall, though it doesn't play well with 
> clang-format since this is a .c file. Should we just ignore clang-format 
> complaints for this file?
Fixed the formatting by adding a `clang-format off` directive at the top of 
this file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87047

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

[PATCH] D87047: [clang] Add command line options for the Machine Function Splitter.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish updated this revision to Diff 291122.
snehasish marked an inline comment as done.
snehasish added a comment.

Fix test formatting.

Added clang-format off to disable clang format for the test which contains both 
profile data (as text) and c code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87047

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/split-machine-functions.c
  clang/test/Driver/fsplit-machine-functions.c

Index: clang/test/Driver/fsplit-machine-functions.c
===
--- /dev/null
+++ clang/test/Driver/fsplit-machine-functions.c
@@ -0,0 +1,9 @@
+// RUN: %clang -### -target x86_64 -fsplit-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
+// RUN: %clang -### -target x86_64 -fsplit-machine-functions -fno-split-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-NOOPT %s
+// RUN: %clang -c -target x86_64 -fsplit-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-WARN %s
+// RUN: not %clang -c -target arm-unknown-linux -fsplit-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s
+
+// CHECK-OPT:   "-fsplit-machine-functions"
+// CHECK-NOOPT-NOT: "-fsplit-machine-functions"
+// CHECK-WARN:  ignored -fsplit-machine-functions, no profile provided via -fprofile-use
+// CHECK-TRIPLE:error: unsupported option '-fsplit-machine-functions' for target
Index: clang/test/CodeGen/split-machine-functions.c
===
--- /dev/null
+++ clang/test/CodeGen/split-machine-functions.c
@@ -0,0 +1,38 @@
+// clang-format off
+// REQUIRES: x86-registered-target
+// RUN: split-file %s %t
+// RUN: llvm-profdata merge -o %t/default.profdata %t/proftext
+// RUN: %clang_cc1 -triple x86_64 -O3 -S -fprofile-instrument-use-path=%t/default.profdata -fsplit-machine-functions -o - < %t/code | FileCheck %s
+
+// CHECK: .section .text.hot.,"ax",@progbits
+// CHECK: foo:
+// CHECK: section .text.unlikely.foo,"ax",@progbits
+// CHECK: foo.cold:
+// CHECK: callq exit@PLT
+
+//--- proftext
+foo
+# Func Hash:
+11262309905
+# Num Counters:
+2
+# Counter Values:
+100
+0
+
+//--- code
+__attribute__((noinline)) int foo(int argc) {
+  if (argc % 2 == 0) {
+exit(argc);
+  } else {
+return argc + 1;
+  }
+}
+
+int main(int argc, char *argv[]) {
+  int total = 0;
+  for (int i = 0; i < 100; ++i) {
+total += foo(argc);
+  }
+  printf("%d\n", total);
+}
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -998,6 +998,8 @@
   Opts.UniqueInternalLinkageNames =
   Args.hasArg(OPT_funique_internal_linkage_names);
 
+  Opts.SplitMachineFunctions = Args.hasArg(OPT_fsplit_machine_functions);
+
   Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
 
   Opts.NoUseJumpTables = Args.hasArg(OPT_fno_jump_tables);
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4911,6 +4911,15 @@
options::OPT_fno_unique_basic_block_section_names, false))
 CmdArgs.push_back("-funique-basic-block-section-names");
 
+  if (Arg *A = Args.getLastArg(options::OPT_fsplit_machine_functions,
+   options::OPT_fno_split_machine_functions)) {
+if (Triple.isX86() && Triple.isOSBinFormatELF())
+  A->render(Args, CmdArgs);
+else
+  D.Diag(diag::err_drv_unsupported_opt_for_target)
+  << A->getAsString(Args) << TripleStr;
+  }
+
   Args.AddLastArg(CmdArgs, options::OPT_finstrument_functions,
   options::OPT_finstrument_functions_after_inlining,
   options::OPT_finstrument_function_entry_bare);
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -9,6 +9,7 @@
 #include "clang/CodeGen/BackendUtil.h"
 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/DiagnosticFrontend.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/TargetOptions.h"
 #include "clang/Frontend/FrontendDiagnostic.h"
@@ -514,6 +515,15 @@
   Options.BBSectionsFuncListBuf = std::move(*MBOrErr);
   }
 
+  if (CodeGenOpts.SplitMachineFunctions) {
+if (CodeGenOpts.getProfileUse() != CodeGenOptions::ProfileNone)
+  Options.EnableMachineFunctionSplitter = true;
+else
+  

[PATCH] D87047: [clang] Add command line options for the Machine Function Splitter.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish marked 3 inline comments as done.
snehasish added a comment.

PTAL, thanks!




Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4259
 options::OPT_fno_unique_basic_block_section_names,
+options::OPT_fsplit_machine_functions,
+options::OPT_fno_split_machine_functions,

MaskRay wrote:
> This is not needed.
> 
> This is for fembed-bitcode and people seem to randomly add options here. Many 
> options are probably not needed.
Thanks for catching this. I read this list as "options which should be ignored 
for embedding bitcode". In this case we do want to save this flag to pass back 
to clang if necessary (as mtrofin@ pointed out offline). Perhaps the 
documentation around this could be enhanced.



Comment at: clang/test/CodeGen/split-machine-functions.c:3
+
+// RUN: echo "foo"> %t.proftext
+// RUN: echo "# Func Hash:"   >> %t.proftext

MaskRay wrote:
> Consider `RUN: split-file`
> 
> Search for this string for some examples.
Looks much cleaner with split-file overall, though it doesn't play well with 
clang-format since this is a .c file. Should we just ignore clang-format 
complaints for this file?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87047

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

[PATCH] D87047: [clang] Add command line options for the Machine Function Splitter.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish updated this revision to Diff 291108.
snehasish added a comment.

Use OptInFFlag, split-file and update tests.

- Change the flag type to OptInFFlag.
- Use split-file in the test to avoid "RUN: echo" lines.
- Use an existing warn message (if no profile is available) and add a check for 
it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87047

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/split-machine-functions.c
  clang/test/Driver/fsplit-machine-functions.c

Index: clang/test/Driver/fsplit-machine-functions.c
===
--- /dev/null
+++ clang/test/Driver/fsplit-machine-functions.c
@@ -0,0 +1,9 @@
+// RUN: %clang -### -target x86_64 -fsplit-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
+// RUN: %clang -### -target x86_64 -fsplit-machine-functions -fno-split-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-NOOPT %s
+// RUN: %clang -c -target x86_64 -fsplit-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-WARN %s
+// RUN: not %clang -c -target arm-unknown-linux -fsplit-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s
+
+// CHECK-OPT:   "-fsplit-machine-functions"
+// CHECK-NOOPT-NOT: "-fsplit-machine-functions"
+// CHECK-WARN:  ignored -fsplit-machine-functions, no profile provided via -fprofile-use
+// CHECK-TRIPLE:error: unsupported option '-fsplit-machine-functions' for target
Index: clang/test/CodeGen/split-machine-functions.c
===
--- /dev/null
+++ clang/test/CodeGen/split-machine-functions.c
@@ -0,0 +1,37 @@
+// REQUIRES: x86-registered-target
+// RUN: split-file %s %t
+// RUN: llvm-profdata merge -o %t/default.profdata %t/proftext
+// RUN: %clang_cc1 -triple x86_64 -O3 -S -fprofile-instrument-use-path=%t/default.profdata -fsplit-machine-functions -o - < %t/code | FileCheck %s
+
+// CHECK: .section .text.hot.,"ax",@progbits
+// CHECK: foo:
+// CHECK: section .text.unlikely.foo,"ax",@progbits
+// CHECK: foo.cold:
+// CHECK: callq exit@PLT
+
+//--- proftext
+foo
+#Func Hash:
+11262309905
+#Num Counters:
+2
+#Counter Values:
+100 0
+
+//--- code
+__attribute__((noinline)) int
+foo(int argc) {
+  if (argc % 2 == 0) {
+exit(argc);
+  } else {
+return argc + 1;
+  }
+}
+
+int main(int argc, char *argv[]) {
+  int total = 0;
+  for (int i = 0; i < 100; ++i) {
+total += foo(argc);
+  }
+  printf("%d\n", total);
+}
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -998,6 +998,8 @@
   Opts.UniqueInternalLinkageNames =
   Args.hasArg(OPT_funique_internal_linkage_names);
 
+  Opts.SplitMachineFunctions = Args.hasArg(OPT_fsplit_machine_functions);
+
   Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
 
   Opts.NoUseJumpTables = Args.hasArg(OPT_fno_jump_tables);
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4911,6 +4911,15 @@
options::OPT_fno_unique_basic_block_section_names, false))
 CmdArgs.push_back("-funique-basic-block-section-names");
 
+  if (Arg *A = Args.getLastArg(options::OPT_fsplit_machine_functions,
+   options::OPT_fno_split_machine_functions)) {
+if (Triple.isX86() && Triple.isOSBinFormatELF())
+  A->render(Args, CmdArgs);
+else
+  D.Diag(diag::err_drv_unsupported_opt_for_target)
+  << A->getAsString(Args) << TripleStr;
+  }
+
   Args.AddLastArg(CmdArgs, options::OPT_finstrument_functions,
   options::OPT_finstrument_functions_after_inlining,
   options::OPT_finstrument_function_entry_bare);
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -9,6 +9,7 @@
 #include "clang/CodeGen/BackendUtil.h"
 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/DiagnosticFrontend.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/TargetOptions.h"
 #include "clang/Frontend/FrontendDiagnostic.h"
@@ -514,6 +515,15 @@
   Options.BBSectionsFuncListBuf = std::move(*MBOrErr);
   }
 
+  if (CodeGenOpts.SplitMachineFunctions) {
+if (CodeGenOpts.getProfileUse() != CodeGenOptions::ProfileNone)
+  

[PATCH] D87047: [clang] Add command line options for the Machine Function Splitter.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish updated this revision to Diff 291063.
snehasish added a comment.

Check warning, specify target to avoid failures on windows.

- Added a check for warning emitted if no profile is provided.
- Added a triple for the remaining checks since we now emit an error for 
incompatible targets.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87047

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/split-machine-functions.c
  clang/test/Driver/fsplit-machine-functions.c

Index: clang/test/Driver/fsplit-machine-functions.c
===
--- /dev/null
+++ clang/test/Driver/fsplit-machine-functions.c
@@ -0,0 +1,9 @@
+// RUN: %clang -### -target x86_64 -fsplit-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
+// RUN: %clang -### -target x86_64 -fsplit-machine-functions -fno-split-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-NOOPT %s
+// RUN: %clang -c -target x86_64 -fsplit-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-WARN %s
+// RUN: not %clang -c -target arm-unknown-linux -fsplit-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s
+
+// CHECK-OPT:   "-fsplit-machine-functions"
+// CHECK-NOOPT-NOT: "-fsplit-machine-functions"
+// CHECK-WARN:  ignored -fsplit-machine-functions, no profile provided via -fprofile-use
+// CHECK-TRIPLE:error: unsupported option '-fsplit-machine-functions' for target
Index: clang/test/CodeGen/split-machine-functions.c
===
--- /dev/null
+++ clang/test/CodeGen/split-machine-functions.c
@@ -0,0 +1,34 @@
+// REQUIRES: x86-registered-target
+
+// RUN: echo "foo"> %t.proftext
+// RUN: echo "# Func Hash:"   >> %t.proftext
+// RUN: echo "11262309905">> %t.proftext
+// RUN: echo "# Num Counters:">> %t.proftext
+// RUN: echo "2"  >> %t.proftext
+// RUN: echo "# Counter Values:"  >> %t.proftext
+// RUN: echo "100">> %t.proftext
+// RUN: echo "0"  >> %t.proftext
+// RUN: llvm-profdata merge -o %t.profdata %t.proftext
+// RUN: %clang_cc1 -triple x86_64 -O3 -S -fprofile-instrument-use-path=%t.profdata -fsplit-machine-functions -o - < %s | FileCheck %s
+
+__attribute__((noinline)) int foo(int argc) {
+  if (argc % 2 == 0) {
+exit(argc);
+  } else {
+return argc + 1;
+  }
+}
+
+int main(int argc, char *argv[]) {
+  int total = 0;
+  for (int i = 0; i < 100; ++i) {
+total += foo(argc);
+  }
+  printf("%d\n", total);
+}
+
+// CHECK: .section .text.hot.,"ax",@progbits
+// CHECK: foo:
+// CHECK: section .text.unlikely.foo,"ax",@progbits
+// CHECK: foo.cold:
+// CHECK: callq exit@PLT
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -998,6 +998,8 @@
   Opts.UniqueInternalLinkageNames =
   Args.hasArg(OPT_funique_internal_linkage_names);
 
+  Opts.SplitMachineFunctions = Args.hasArg(OPT_fsplit_machine_functions);
+
   Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
 
   Opts.NoUseJumpTables = Args.hasArg(OPT_fno_jump_tables);
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4256,6 +4256,8 @@
 options::OPT_fno_unique_section_names,
 options::OPT_funique_basic_block_section_names,
 options::OPT_fno_unique_basic_block_section_names,
+options::OPT_fsplit_machine_functions,
+options::OPT_fno_split_machine_functions,
 options::OPT_mrestrict_it,
 options::OPT_mno_restrict_it,
 options::OPT_mstackrealign,
@@ -4911,6 +4913,15 @@
options::OPT_fno_unique_basic_block_section_names, false))
 CmdArgs.push_back("-funique-basic-block-section-names");
 
+  if (Args.hasFlag(options::OPT_fsplit_machine_functions,
+   options::OPT_fno_split_machine_functions, false)) {
+if (Triple.isX86() && Triple.isOSBinFormatELF())
+  CmdArgs.push_back("-fsplit-machine-functions");
+else
+  D.Diag(diag::err_drv_unsupported_opt_for_target)
+  << "-fsplit-machine-functions" << TripleStr;
+  }
+
   Args.AddLastArg(CmdArgs, options::OPT_finstrument_functions,
   options::OPT_finstrument_functions_after_inlining,
   options::OPT_finstrument_function_entry_bare);
Index: clang/lib/CodeGen/BackendUtil.cpp

[PATCH] D87047: [clang] Add command line options for the Machine Function Splitter.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish updated this revision to Diff 291051.
snehasish added a comment.

Updated test and warning type.

- Updated test to check for the diagnostic message.
- Updated BackendUtil.cpp to use backend optimization failure warning.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87047

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/split-machine-functions.c
  clang/test/Driver/fsplit-machine-functions.c

Index: clang/test/Driver/fsplit-machine-functions.c
===
--- /dev/null
+++ clang/test/Driver/fsplit-machine-functions.c
@@ -0,0 +1,7 @@
+// RUN: %clang -### -fsplit-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
+// RUN: %clang -### -fsplit-machine-functions -fno-split-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-NOOPT %s
+// RUN: not %clang -c -target arm-unknown-linux -fsplit-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s
+
+// CHECK-OPT:   "-fsplit-machine-functions"
+// CHECK-NOOPT-NOT: "-fsplit-machine-functions"
+// CHECK-TRIPLE:error: unsupported option '-fsplit-machine-functions' for target
Index: clang/test/CodeGen/split-machine-functions.c
===
--- /dev/null
+++ clang/test/CodeGen/split-machine-functions.c
@@ -0,0 +1,34 @@
+// REQUIRES: x86-registered-target
+
+// RUN: echo "foo"> %t.proftext
+// RUN: echo "# Func Hash:"   >> %t.proftext
+// RUN: echo "11262309905">> %t.proftext
+// RUN: echo "# Num Counters:">> %t.proftext
+// RUN: echo "2"  >> %t.proftext
+// RUN: echo "# Counter Values:"  >> %t.proftext
+// RUN: echo "100">> %t.proftext
+// RUN: echo "0"  >> %t.proftext
+// RUN: llvm-profdata merge -o %t.profdata %t.proftext
+// RUN: %clang_cc1 -triple x86_64 -O3 -S -fprofile-instrument-use-path=%t.profdata -fsplit-machine-functions -o - < %s | FileCheck %s
+
+__attribute__((noinline)) int foo(int argc) {
+  if (argc % 2 == 0) {
+exit(argc);
+  } else {
+return argc + 1;
+  }
+}
+
+int main(int argc, char *argv[]) {
+  int total = 0;
+  for (int i = 0; i < 100; ++i) {
+total += foo(argc);
+  }
+  printf("%d\n", total);
+}
+
+// CHECK: .section .text.hot.,"ax",@progbits
+// CHECK: foo:
+// CHECK: section .text.unlikely.foo,"ax",@progbits
+// CHECK: foo.cold:
+// CHECK: callq exit@PLT
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -998,6 +998,8 @@
   Opts.UniqueInternalLinkageNames =
   Args.hasArg(OPT_funique_internal_linkage_names);
 
+  Opts.SplitMachineFunctions = Args.hasArg(OPT_fsplit_machine_functions);
+
   Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
 
   Opts.NoUseJumpTables = Args.hasArg(OPT_fno_jump_tables);
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4256,6 +4256,8 @@
 options::OPT_fno_unique_section_names,
 options::OPT_funique_basic_block_section_names,
 options::OPT_fno_unique_basic_block_section_names,
+options::OPT_fsplit_machine_functions,
+options::OPT_fno_split_machine_functions,
 options::OPT_mrestrict_it,
 options::OPT_mno_restrict_it,
 options::OPT_mstackrealign,
@@ -4911,6 +4913,15 @@
options::OPT_fno_unique_basic_block_section_names, false))
 CmdArgs.push_back("-funique-basic-block-section-names");
 
+  if (Args.hasFlag(options::OPT_fsplit_machine_functions,
+   options::OPT_fno_split_machine_functions, false)) {
+if (Triple.isX86() && Triple.isOSBinFormatELF())
+  CmdArgs.push_back("-fsplit-machine-functions");
+else
+  D.Diag(diag::err_drv_unsupported_opt_for_target)
+  << "-fsplit-machine-functions" << TripleStr;
+  }
+
   Args.AddLastArg(CmdArgs, options::OPT_finstrument_functions,
   options::OPT_finstrument_functions_after_inlining,
   options::OPT_finstrument_function_entry_bare);
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -9,6 +9,7 @@
 #include "clang/CodeGen/BackendUtil.h"
 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/DiagnosticFrontend.h"
 #include "clang/Basic/LangOptions.h"
 

[PATCH] D87426: Disallow fbasic-block-sections on non-ELF, non-x86 targets.

[Snehasish Kumar via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG157cd93b48a9: [clang] Disallow fbasic-block-sections on 
non-ELF, non-x86 targets. (authored by snehasish).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87426

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/fbasic-block-sections.c


Index: clang/test/Driver/fbasic-block-sections.c
===
--- clang/test/Driver/fbasic-block-sections.c
+++ clang/test/Driver/fbasic-block-sections.c
@@ -1,9 +1,12 @@
-// RUN: %clang -### -fbasic-block-sections=none %s -S 2>&1 | FileCheck 
-check-prefix=CHECK-OPT-NONE %s
-// RUN: %clang -### -fbasic-block-sections=all %s -S 2>&1 | FileCheck 
-check-prefix=CHECK-OPT-ALL %s
-// RUN: %clang -### -fbasic-block-sections=list=%s %s -S 2>&1 | FileCheck 
-check-prefix=CHECK-OPT-LIST %s
-// RUN: %clang -### -fbasic-block-sections=labels %s -S 2>&1 | FileCheck 
-check-prefix=CHECK-OPT-LABELS %s
+// RUN: %clang -### -target x86_64 -fbasic-block-sections=none %s -S 2>&1 | 
FileCheck -check-prefix=CHECK-OPT-NONE %s
+// RUN: %clang -### -target x86_64 -fbasic-block-sections=all %s -S 2>&1 | 
FileCheck -check-prefix=CHECK-OPT-ALL %s
+// RUN: %clang -### -target x86_64 -fbasic-block-sections=list=%s %s -S 2>&1 | 
FileCheck -check-prefix=CHECK-OPT-LIST %s
+// RUN: %clang -### -target x86_64 -fbasic-block-sections=labels %s -S 2>&1 | 
FileCheck -check-prefix=CHECK-OPT-LABELS %s
+// RUN: not %clang -c -target arm-unknown-linux -fbasic-block-sections=all %s 
-S 2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s
+// RUN: not %clang -c -target x86_64-apple-darwin10 -fbasic-block-sections=all 
%s -S 2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s
 //
-// CHECK-OPT-NONE: "-fbasic-block-sections=none"
-// CHECK-OPT-ALL: "-fbasic-block-sections=all"
-// CHECK-OPT-LIST: "-fbasic-block-sections={{[^ ]*}}fbasic-block-sections.c"
+// CHECK-OPT-NONE:   "-fbasic-block-sections=none"
+// CHECK-OPT-ALL:"-fbasic-block-sections=all"
+// CHECK-OPT-LIST:   "-fbasic-block-sections={{[^ ]*}}fbasic-block-sections.c"
 // CHECK-OPT-LABELS: "-fbasic-block-sections=labels"
+// CHECK-TRIPLE: error: unsupported option '-fbasic-block-sections=all' 
for target
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4880,13 +4880,18 @@
   }
 
   if (Arg *A = Args.getLastArg(options::OPT_fbasic_block_sections_EQ)) {
-StringRef Val = A->getValue();
-if (Val != "all" && Val != "labels" && Val != "none" &&
-!(Val.startswith("list=") && llvm::sys::fs::exists(Val.substr(5
-  D.Diag(diag::err_drv_invalid_value)
-  << A->getAsString(Args) << A->getValue();
-else
-  A->render(Args, CmdArgs);
+if (Triple.isX86() && Triple.isOSBinFormatELF()) {
+  StringRef Val = A->getValue();
+  if (Val != "all" && Val != "labels" && Val != "none" &&
+  !(Val.startswith("list=") && llvm::sys::fs::exists(Val.substr(5
+D.Diag(diag::err_drv_invalid_value)
+<< A->getAsString(Args) << A->getValue();
+  else
+A->render(Args, CmdArgs);
+} else {
+  D.Diag(diag::err_drv_unsupported_opt_for_target)
+  << A->getAsString(Args) << TripleStr;
+}
   }
 
   if (Args.hasFlag(options::OPT_fdata_sections, options::OPT_fno_data_sections,


Index: clang/test/Driver/fbasic-block-sections.c
===
--- clang/test/Driver/fbasic-block-sections.c
+++ clang/test/Driver/fbasic-block-sections.c
@@ -1,9 +1,12 @@
-// RUN: %clang -### -fbasic-block-sections=none %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-NONE %s
-// RUN: %clang -### -fbasic-block-sections=all %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-ALL %s
-// RUN: %clang -### -fbasic-block-sections=list=%s %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-LIST %s
-// RUN: %clang -### -fbasic-block-sections=labels %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-LABELS %s
+// RUN: %clang -### -target x86_64 -fbasic-block-sections=none %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-NONE %s
+// RUN: %clang -### -target x86_64 -fbasic-block-sections=all %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-ALL %s
+// RUN: %clang -### -target x86_64 -fbasic-block-sections=list=%s %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-LIST %s
+// RUN: %clang -### -target x86_64 -fbasic-block-sections=labels %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-LABELS %s
+// RUN: not %clang -c -target arm-unknown-linux -fbasic-block-sections=all %s -S 2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s
+// RUN: not %clang -c -target x86_64-apple-darwin10 -fbasic-block-sections=all %s -S 2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s

[PATCH] D87426: Disallow fbasic-block-sections on non-ELF, non-x86 targets.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish marked 4 inline comments as done.
snehasish added a comment.

Thanks for explaining the rationale, PTAL.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87426

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

[PATCH] D87426: Disallow fbasic-block-sections on non-ELF, non-x86 targets.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish updated this revision to Diff 290884.
snehasish marked an inline comment as done.
snehasish added a comment.

Update test to use `not` tool and `-c` flag.

- Use the not tool to indicate failure is expected.
- Use -c instead of -### for failing invocations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87426

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/fbasic-block-sections.c


Index: clang/test/Driver/fbasic-block-sections.c
===
--- clang/test/Driver/fbasic-block-sections.c
+++ clang/test/Driver/fbasic-block-sections.c
@@ -1,9 +1,12 @@
-// RUN: %clang -### -fbasic-block-sections=none %s -S 2>&1 | FileCheck 
-check-prefix=CHECK-OPT-NONE %s
-// RUN: %clang -### -fbasic-block-sections=all %s -S 2>&1 | FileCheck 
-check-prefix=CHECK-OPT-ALL %s
-// RUN: %clang -### -fbasic-block-sections=list=%s %s -S 2>&1 | FileCheck 
-check-prefix=CHECK-OPT-LIST %s
-// RUN: %clang -### -fbasic-block-sections=labels %s -S 2>&1 | FileCheck 
-check-prefix=CHECK-OPT-LABELS %s
+// RUN: %clang -### -target x86_64 -fbasic-block-sections=none %s -S 2>&1 | 
FileCheck -check-prefix=CHECK-OPT-NONE %s
+// RUN: %clang -### -target x86_64 -fbasic-block-sections=all %s -S 2>&1 | 
FileCheck -check-prefix=CHECK-OPT-ALL %s
+// RUN: %clang -### -target x86_64 -fbasic-block-sections=list=%s %s -S 2>&1 | 
FileCheck -check-prefix=CHECK-OPT-LIST %s
+// RUN: %clang -### -target x86_64 -fbasic-block-sections=labels %s -S 2>&1 | 
FileCheck -check-prefix=CHECK-OPT-LABELS %s
+// RUN: not %clang -c -target arm-unknown-linux -fbasic-block-sections=all %s 
-S 2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s
+// RUN: not %clang -c -target x86_64-apple-darwin10 -fbasic-block-sections=all 
%s -S 2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s
 //
-// CHECK-OPT-NONE: "-fbasic-block-sections=none"
-// CHECK-OPT-ALL: "-fbasic-block-sections=all"
-// CHECK-OPT-LIST: "-fbasic-block-sections={{[^ ]*}}fbasic-block-sections.c"
+// CHECK-OPT-NONE:   "-fbasic-block-sections=none"
+// CHECK-OPT-ALL:"-fbasic-block-sections=all"
+// CHECK-OPT-LIST:   "-fbasic-block-sections={{[^ ]*}}fbasic-block-sections.c"
 // CHECK-OPT-LABELS: "-fbasic-block-sections=labels"
+// CHECK-TRIPLE: error: unsupported option '-fbasic-block-sections=all' 
for target
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4880,13 +4880,18 @@
   }
 
   if (Arg *A = Args.getLastArg(options::OPT_fbasic_block_sections_EQ)) {
-StringRef Val = A->getValue();
-if (Val != "all" && Val != "labels" && Val != "none" &&
-!(Val.startswith("list=") && llvm::sys::fs::exists(Val.substr(5
-  D.Diag(diag::err_drv_invalid_value)
-  << A->getAsString(Args) << A->getValue();
-else
-  A->render(Args, CmdArgs);
+if (Triple.isX86() && Triple.isOSBinFormatELF()) {
+  StringRef Val = A->getValue();
+  if (Val != "all" && Val != "labels" && Val != "none" &&
+  !(Val.startswith("list=") && llvm::sys::fs::exists(Val.substr(5
+D.Diag(diag::err_drv_invalid_value)
+<< A->getAsString(Args) << A->getValue();
+  else
+A->render(Args, CmdArgs);
+} else {
+  D.Diag(diag::err_drv_unsupported_opt_for_target)
+  << A->getAsString(Args) << TripleStr;
+}
   }
 
   if (Args.hasFlag(options::OPT_fdata_sections, options::OPT_fno_data_sections,


Index: clang/test/Driver/fbasic-block-sections.c
===
--- clang/test/Driver/fbasic-block-sections.c
+++ clang/test/Driver/fbasic-block-sections.c
@@ -1,9 +1,12 @@
-// RUN: %clang -### -fbasic-block-sections=none %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-NONE %s
-// RUN: %clang -### -fbasic-block-sections=all %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-ALL %s
-// RUN: %clang -### -fbasic-block-sections=list=%s %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-LIST %s
-// RUN: %clang -### -fbasic-block-sections=labels %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-LABELS %s
+// RUN: %clang -### -target x86_64 -fbasic-block-sections=none %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-NONE %s
+// RUN: %clang -### -target x86_64 -fbasic-block-sections=all %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-ALL %s
+// RUN: %clang -### -target x86_64 -fbasic-block-sections=list=%s %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-LIST %s
+// RUN: %clang -### -target x86_64 -fbasic-block-sections=labels %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-LABELS %s
+// RUN: not %clang -c -target arm-unknown-linux -fbasic-block-sections=all %s -S 2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s
+// RUN: not %clang -c -target x86_64-apple-darwin10 -fbasic-block-sections=all %s -S 2>&1 | FileCheck 

[PATCH] D87426: Disallow fbasic-block-sections on non-ELF, non-x86 targets.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish marked 3 inline comments as done.
snehasish added a comment.

Thanks for the quick review @MaskRay, PTAL.




Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4892
+} else {
+  D.Diag(diag::warn_drv_unsupported_opt_for_target)
+  << A->getAsString(Args) << TripleStr;

MaskRay wrote:
> This should be an error
Sure, can you provide a reference/guidance for when err vs warn is appropriate 
for driver checks? Most of the code in this file uses err but there are some 
cases of warn being used.



Comment at: clang/test/Driver/fbasic-block-sections.c:5
+// RUN: %clang -### -target x86_64-linux-gnu -fbasic-block-sections=labels %s 
-S 2>&1 | FileCheck -check-prefix=CHECK-OPT-LABELS %s
+// RUN: %clang -### -target arm-unknown-linux -fbasic-block-sections=all %s -S 
2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s
+// RUN: %clang -### -target x86_64-apple-darwin10 -fbasic-block-sections=all 
%s -S 2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s

MaskRay wrote:
> `%clang -###` -> `not %clang -fsyntax-only`
What I really want to check for is the absence of the flag for the particular 
triple. Using -fsyntax-only means that the cc1 args are not emitted. I've added 
a check for the diagnostic message but retained `-###` so that I can keep the 
check for the absence of `-fbasic-block-sections=all`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87426

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

[PATCH] D87426: Disallow fbasic-block-sections on non-ELF, non-x86 targets.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish updated this revision to Diff 290867.
snehasish added a comment.

Update test based on review comments.

- Use diag::err instead of warn.
- Drop linux-gnu from the triple for the default test.
- Check for the presence of the diagnostic message.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87426

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/fbasic-block-sections.c


Index: clang/test/Driver/fbasic-block-sections.c
===
--- clang/test/Driver/fbasic-block-sections.c
+++ clang/test/Driver/fbasic-block-sections.c
@@ -1,9 +1,13 @@
-// RUN: %clang -### -fbasic-block-sections=none %s -S 2>&1 | FileCheck 
-check-prefix=CHECK-OPT-NONE %s
-// RUN: %clang -### -fbasic-block-sections=all %s -S 2>&1 | FileCheck 
-check-prefix=CHECK-OPT-ALL %s
-// RUN: %clang -### -fbasic-block-sections=list=%s %s -S 2>&1 | FileCheck 
-check-prefix=CHECK-OPT-LIST %s
-// RUN: %clang -### -fbasic-block-sections=labels %s -S 2>&1 | FileCheck 
-check-prefix=CHECK-OPT-LABELS %s
+// RUN: %clang -### -target x86_64 -fbasic-block-sections=none %s -S 2>&1 | 
FileCheck -check-prefix=CHECK-OPT-NONE %s
+// RUN: %clang -### -target x86_64 -fbasic-block-sections=all %s -S 2>&1 | 
FileCheck -check-prefix=CHECK-OPT-ALL %s
+// RUN: %clang -### -target x86_64 -fbasic-block-sections=list=%s %s -S 2>&1 | 
FileCheck -check-prefix=CHECK-OPT-LIST %s
+// RUN: %clang -### -target x86_64 -fbasic-block-sections=labels %s -S 2>&1 | 
FileCheck -check-prefix=CHECK-OPT-LABELS %s
+// RUN: %clang -### -target arm-unknown-linux -fbasic-block-sections=all %s -S 
2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s
+// RUN: %clang -### -target x86_64-apple-darwin10 -fbasic-block-sections=all 
%s -S 2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s
 //
-// CHECK-OPT-NONE: "-fbasic-block-sections=none"
-// CHECK-OPT-ALL: "-fbasic-block-sections=all"
-// CHECK-OPT-LIST: "-fbasic-block-sections={{[^ ]*}}fbasic-block-sections.c"
+// CHECK-OPT-NONE:   "-fbasic-block-sections=none"
+// CHECK-OPT-ALL:"-fbasic-block-sections=all"
+// CHECK-OPT-LIST:   "-fbasic-block-sections={{[^ ]*}}fbasic-block-sections.c"
 // CHECK-OPT-LABELS: "-fbasic-block-sections=labels"
+// CHECK-TRIPLE-NOT: "-fbasic-block-sections=all"
+// CHECK-TRIPLE: error: unsupported option '-fbasic-block-sections=all' 
for target
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4880,13 +4880,18 @@
   }
 
   if (Arg *A = Args.getLastArg(options::OPT_fbasic_block_sections_EQ)) {
-StringRef Val = A->getValue();
-if (Val != "all" && Val != "labels" && Val != "none" &&
-!(Val.startswith("list=") && llvm::sys::fs::exists(Val.substr(5
-  D.Diag(diag::err_drv_invalid_value)
-  << A->getAsString(Args) << A->getValue();
-else
-  A->render(Args, CmdArgs);
+if (Triple.isX86() && Triple.isOSBinFormatELF()) {
+  StringRef Val = A->getValue();
+  if (Val != "all" && Val != "labels" && Val != "none" &&
+  !(Val.startswith("list=") && llvm::sys::fs::exists(Val.substr(5
+D.Diag(diag::err_drv_invalid_value)
+<< A->getAsString(Args) << A->getValue();
+  else
+A->render(Args, CmdArgs);
+} else {
+  D.Diag(diag::err_drv_unsupported_opt_for_target)
+  << A->getAsString(Args) << TripleStr;
+}
   }
 
   if (Args.hasFlag(options::OPT_fdata_sections, options::OPT_fno_data_sections,


Index: clang/test/Driver/fbasic-block-sections.c
===
--- clang/test/Driver/fbasic-block-sections.c
+++ clang/test/Driver/fbasic-block-sections.c
@@ -1,9 +1,13 @@
-// RUN: %clang -### -fbasic-block-sections=none %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-NONE %s
-// RUN: %clang -### -fbasic-block-sections=all %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-ALL %s
-// RUN: %clang -### -fbasic-block-sections=list=%s %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-LIST %s
-// RUN: %clang -### -fbasic-block-sections=labels %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-LABELS %s
+// RUN: %clang -### -target x86_64 -fbasic-block-sections=none %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-NONE %s
+// RUN: %clang -### -target x86_64 -fbasic-block-sections=all %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-ALL %s
+// RUN: %clang -### -target x86_64 -fbasic-block-sections=list=%s %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-LIST %s
+// RUN: %clang -### -target x86_64 -fbasic-block-sections=labels %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-LABELS %s
+// RUN: %clang -### -target arm-unknown-linux -fbasic-block-sections=all %s -S 2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s
+// RUN: %clang -### -target x86_64-apple-darwin10 -fbasic-block-sections=all %s -S 2>&1 

[PATCH] D87426: Disallow fbasic-block-sections on non-ELF, non-x86 targets.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish updated this revision to Diff 290863.
snehasish added a comment.

Specify triple for driver tests, address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87426

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/fbasic-block-sections.c


Index: clang/test/Driver/fbasic-block-sections.c
===
--- clang/test/Driver/fbasic-block-sections.c
+++ clang/test/Driver/fbasic-block-sections.c
@@ -1,9 +1,12 @@
-// RUN: %clang -### -fbasic-block-sections=none %s -S 2>&1 | FileCheck 
-check-prefix=CHECK-OPT-NONE %s
-// RUN: %clang -### -fbasic-block-sections=all %s -S 2>&1 | FileCheck 
-check-prefix=CHECK-OPT-ALL %s
-// RUN: %clang -### -fbasic-block-sections=list=%s %s -S 2>&1 | FileCheck 
-check-prefix=CHECK-OPT-LIST %s
-// RUN: %clang -### -fbasic-block-sections=labels %s -S 2>&1 | FileCheck 
-check-prefix=CHECK-OPT-LABELS %s
+// RUN: %clang -### -target x86_64-linux-gnu -fbasic-block-sections=none %s -S 
2>&1 | FileCheck -check-prefix=CHECK-OPT-NONE %s
+// RUN: %clang -### -target x86_64-linux-gnu -fbasic-block-sections=all %s -S 
2>&1 | FileCheck -check-prefix=CHECK-OPT-ALL %s
+// RUN: %clang -### -target x86_64-linux-gnu -fbasic-block-sections=list=%s %s 
-S 2>&1 | FileCheck -check-prefix=CHECK-OPT-LIST %s
+// RUN: %clang -### -target x86_64-linux-gnu -fbasic-block-sections=labels %s 
-S 2>&1 | FileCheck -check-prefix=CHECK-OPT-LABELS %s
+// RUN: %clang -### -target arm-unknown-linux -fbasic-block-sections=all %s -S 
2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s
+// RUN: %clang -### -target x86_64-apple-darwin10 -fbasic-block-sections=all 
%s -S 2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s
 //
-// CHECK-OPT-NONE: "-fbasic-block-sections=none"
-// CHECK-OPT-ALL: "-fbasic-block-sections=all"
-// CHECK-OPT-LIST: "-fbasic-block-sections={{[^ ]*}}fbasic-block-sections.c"
+// CHECK-OPT-NONE:   "-fbasic-block-sections=none"
+// CHECK-OPT-ALL:"-fbasic-block-sections=all"
+// CHECK-OPT-LIST:   "-fbasic-block-sections={{[^ ]*}}fbasic-block-sections.c"
 // CHECK-OPT-LABELS: "-fbasic-block-sections=labels"
+// CHECK-TRIPLE-NOT: "-fbasic-block-sections=all"
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4880,13 +4880,18 @@
   }
 
   if (Arg *A = Args.getLastArg(options::OPT_fbasic_block_sections_EQ)) {
-StringRef Val = A->getValue();
-if (Val != "all" && Val != "labels" && Val != "none" &&
-!(Val.startswith("list=") && llvm::sys::fs::exists(Val.substr(5
-  D.Diag(diag::err_drv_invalid_value)
-  << A->getAsString(Args) << A->getValue();
-else
-  A->render(Args, CmdArgs);
+if (Triple.isX86() && Triple.isOSBinFormatELF()) {
+  StringRef Val = A->getValue();
+  if (Val != "all" && Val != "labels" && Val != "none" &&
+  !(Val.startswith("list=") && llvm::sys::fs::exists(Val.substr(5
+D.Diag(diag::err_drv_invalid_value)
+<< A->getAsString(Args) << A->getValue();
+  else
+A->render(Args, CmdArgs);
+} else {
+  D.Diag(diag::warn_drv_unsupported_opt_for_target)
+  << A->getAsString(Args) << TripleStr;
+}
   }
 
   if (Args.hasFlag(options::OPT_fdata_sections, options::OPT_fno_data_sections,


Index: clang/test/Driver/fbasic-block-sections.c
===
--- clang/test/Driver/fbasic-block-sections.c
+++ clang/test/Driver/fbasic-block-sections.c
@@ -1,9 +1,12 @@
-// RUN: %clang -### -fbasic-block-sections=none %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-NONE %s
-// RUN: %clang -### -fbasic-block-sections=all %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-ALL %s
-// RUN: %clang -### -fbasic-block-sections=list=%s %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-LIST %s
-// RUN: %clang -### -fbasic-block-sections=labels %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-LABELS %s
+// RUN: %clang -### -target x86_64-linux-gnu -fbasic-block-sections=none %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-NONE %s
+// RUN: %clang -### -target x86_64-linux-gnu -fbasic-block-sections=all %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-ALL %s
+// RUN: %clang -### -target x86_64-linux-gnu -fbasic-block-sections=list=%s %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-LIST %s
+// RUN: %clang -### -target x86_64-linux-gnu -fbasic-block-sections=labels %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-LABELS %s
+// RUN: %clang -### -target arm-unknown-linux -fbasic-block-sections=all %s -S 2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s
+// RUN: %clang -### -target x86_64-apple-darwin10 -fbasic-block-sections=all %s -S 2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s
 //
-// CHECK-OPT-NONE: "-fbasic-block-sections=none"
-// CHECK-OPT-ALL: 

[PATCH] D87426: Disallow fbasic-block-sections on non-ELF, non-x86 targets.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish created this revision.
snehasish added reviewers: tmsriram, rahmanl.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
snehasish requested review of this revision.

Basic block sections is untested on other platforms and binary formats apart 
from x86,elf. This patch silently drops the flag if the platform and binary 
format is not compatible.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D87426

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/fbasic-block-sections.c


Index: clang/test/Driver/fbasic-block-sections.c
===
--- clang/test/Driver/fbasic-block-sections.c
+++ clang/test/Driver/fbasic-block-sections.c
@@ -2,8 +2,10 @@
 // RUN: %clang -### -fbasic-block-sections=all %s -S 2>&1 | FileCheck 
-check-prefix=CHECK-OPT-ALL %s
 // RUN: %clang -### -fbasic-block-sections=list=%s %s -S 2>&1 | FileCheck 
-check-prefix=CHECK-OPT-LIST %s
 // RUN: %clang -### -fbasic-block-sections=labels %s -S 2>&1 | FileCheck 
-check-prefix=CHECK-OPT-LABELS %s
+// RUN: %clang -### -target arm-unknown-linux -fbasic-block-sections=all %s -S 
2>&1 | FileCheck -check-prefix=CHECK-NOOPT %s
 //
-// CHECK-OPT-NONE: "-fbasic-block-sections=none"
-// CHECK-OPT-ALL: "-fbasic-block-sections=all"
-// CHECK-OPT-LIST: "-fbasic-block-sections={{[^ ]*}}fbasic-block-sections.c"
+// CHECK-OPT-NONE:   "-fbasic-block-sections=none"
+// CHECK-OPT-ALL:"-fbasic-block-sections=all"
+// CHECK-OPT-LIST:   "-fbasic-block-sections={{[^ ]*}}fbasic-block-sections.c"
 // CHECK-OPT-LABELS: "-fbasic-block-sections=labels"
+// CHECK-NOOPT-NOT:  "-fbasic-block-sections"
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4879,14 +4879,16 @@
 CmdArgs.push_back("-ffunction-sections");
   }
 
-  if (Arg *A = Args.getLastArg(options::OPT_fbasic_block_sections_EQ)) {
-StringRef Val = A->getValue();
-if (Val != "all" && Val != "labels" && Val != "none" &&
-!(Val.startswith("list=") && llvm::sys::fs::exists(Val.substr(5
-  D.Diag(diag::err_drv_invalid_value)
-  << A->getAsString(Args) << A->getValue();
-else
-  A->render(Args, CmdArgs);
+  if (Triple.isX86() && Triple.isOSBinFormatELF()) {
+if (Arg *A = Args.getLastArg(options::OPT_fbasic_block_sections_EQ)) {
+  StringRef Val = A->getValue();
+  if (Val != "all" && Val != "labels" && Val != "none" &&
+  !(Val.startswith("list=") && llvm::sys::fs::exists(Val.substr(5
+D.Diag(diag::err_drv_invalid_value)
+<< A->getAsString(Args) << A->getValue();
+  else
+A->render(Args, CmdArgs);
+}
   }
 
   if (Args.hasFlag(options::OPT_fdata_sections, options::OPT_fno_data_sections,


Index: clang/test/Driver/fbasic-block-sections.c
===
--- clang/test/Driver/fbasic-block-sections.c
+++ clang/test/Driver/fbasic-block-sections.c
@@ -2,8 +2,10 @@
 // RUN: %clang -### -fbasic-block-sections=all %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-ALL %s
 // RUN: %clang -### -fbasic-block-sections=list=%s %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-LIST %s
 // RUN: %clang -### -fbasic-block-sections=labels %s -S 2>&1 | FileCheck -check-prefix=CHECK-OPT-LABELS %s
+// RUN: %clang -### -target arm-unknown-linux -fbasic-block-sections=all %s -S 2>&1 | FileCheck -check-prefix=CHECK-NOOPT %s
 //
-// CHECK-OPT-NONE: "-fbasic-block-sections=none"
-// CHECK-OPT-ALL: "-fbasic-block-sections=all"
-// CHECK-OPT-LIST: "-fbasic-block-sections={{[^ ]*}}fbasic-block-sections.c"
+// CHECK-OPT-NONE:   "-fbasic-block-sections=none"
+// CHECK-OPT-ALL:"-fbasic-block-sections=all"
+// CHECK-OPT-LIST:   "-fbasic-block-sections={{[^ ]*}}fbasic-block-sections.c"
 // CHECK-OPT-LABELS: "-fbasic-block-sections=labels"
+// CHECK-NOOPT-NOT:  "-fbasic-block-sections"
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4879,14 +4879,16 @@
 CmdArgs.push_back("-ffunction-sections");
   }
 
-  if (Arg *A = Args.getLastArg(options::OPT_fbasic_block_sections_EQ)) {
-StringRef Val = A->getValue();
-if (Val != "all" && Val != "labels" && Val != "none" &&
-!(Val.startswith("list=") && llvm::sys::fs::exists(Val.substr(5
-  D.Diag(diag::err_drv_invalid_value)
-  << A->getAsString(Args) << A->getValue();
-else
-  A->render(Args, CmdArgs);
+  if (Triple.isX86() && Triple.isOSBinFormatELF()) {
+if (Arg *A = Args.getLastArg(options::OPT_fbasic_block_sections_EQ)) {
+  StringRef Val = A->getValue();
+  if (Val != "all" && Val != "labels" && Val != "none" &&
+  !(Val.startswith("list=") && 

[PATCH] D87047: [clang] Add command line options for the Machine Function Splitter.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish updated this revision to Diff 290852.
snehasish added a comment.

Add a check for x86-elf.

- Add a check for the triple in the driver to only enable the flag on x86+elf 
targets.
- Add a test to ensure we don't enable this on other targets.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87047

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/split-machine-functions.c
  clang/test/Driver/fsplit-machine-functions.c

Index: clang/test/Driver/fsplit-machine-functions.c
===
--- /dev/null
+++ clang/test/Driver/fsplit-machine-functions.c
@@ -0,0 +1,5 @@
+// RUN: %clang -### -fsplit-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
+// RUN: %clang -### -fsplit-machine-functions -fno-split-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-NOOPT %s
+// RUN: %clang -### -target arm-unknown-linux -fsplit-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-NOOPT %s
+// CHECK-OPT: "-fsplit-machine-functions"
+// CHECK-NOOPT-NOT: "-fsplit-machine-functions"
Index: clang/test/CodeGen/split-machine-functions.c
===
--- /dev/null
+++ clang/test/CodeGen/split-machine-functions.c
@@ -0,0 +1,34 @@
+// REQUIRES: x86-registered-target
+
+// RUN: echo "foo"> %t.proftext
+// RUN: echo "# Func Hash:"   >> %t.proftext
+// RUN: echo "11262309905">> %t.proftext
+// RUN: echo "# Num Counters:">> %t.proftext
+// RUN: echo "2"  >> %t.proftext
+// RUN: echo "# Counter Values:"  >> %t.proftext
+// RUN: echo "100">> %t.proftext
+// RUN: echo "0"  >> %t.proftext
+// RUN: llvm-profdata merge -o %t.profdata %t.proftext
+// RUN: %clang_cc1 -triple x86_64 -O3 -S -fprofile-instrument-use-path=%t.profdata -fsplit-machine-functions -o - < %s | FileCheck %s
+
+__attribute__((noinline)) int foo(int argc) {
+  if (argc % 2 == 0) {
+exit(argc);
+  } else {
+return argc + 1;
+  }
+}
+
+int main(int argc, char *argv[]) {
+  int total = 0;
+  for (int i = 0; i < 100; ++i) {
+total += foo(argc);
+  }
+  printf("%d\n", total);
+}
+
+// CHECK: .section .text.hot.,"ax",@progbits
+// CHECK: foo:
+// CHECK: section .text.unlikely.foo,"ax",@progbits
+// CHECK: foo.cold:
+// CHECK: callq exit@PLT
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -998,6 +998,8 @@
   Opts.UniqueInternalLinkageNames =
   Args.hasArg(OPT_funique_internal_linkage_names);
 
+  Opts.SplitMachineFunctions = Args.hasArg(OPT_fsplit_machine_functions);
+
   Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
 
   Opts.NoUseJumpTables = Args.hasArg(OPT_fno_jump_tables);
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4256,6 +4256,8 @@
 options::OPT_fno_unique_section_names,
 options::OPT_funique_basic_block_section_names,
 options::OPT_fno_unique_basic_block_section_names,
+options::OPT_fsplit_machine_functions,
+options::OPT_fno_split_machine_functions,
 options::OPT_mrestrict_it,
 options::OPT_mno_restrict_it,
 options::OPT_mstackrealign,
@@ -4906,6 +4908,12 @@
options::OPT_fno_unique_basic_block_section_names, false))
 CmdArgs.push_back("-funique-basic-block-section-names");
 
+  if (Args.hasFlag(options::OPT_fsplit_machine_functions,
+   options::OPT_fno_split_machine_functions, false)) {
+if (Triple.isX86() && Triple.isOSBinFormatELF())
+  CmdArgs.push_back("-fsplit-machine-functions");
+  }
+
   Args.AddLastArg(CmdArgs, options::OPT_finstrument_functions,
   options::OPT_finstrument_functions_after_inlining,
   options::OPT_finstrument_function_entry_bare);
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -514,6 +514,13 @@
   Options.BBSectionsFuncListBuf = std::move(*MBOrErr);
   }
 
+  if (CodeGenOpts.SplitMachineFunctions) {
+if (CodeGenOpts.getProfileUse() != CodeGenOptions::ProfileNone)
+  Options.EnableMachineFunctionSplitter = true;
+else
+  Diags.Report(diag::warn_fe_machine_function_splitter_missing_profile);
+  }
+
   

[PATCH] D87047: [clang] Add command line options for the Machine Function Splitter.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish added a comment.

Thanks for the comments! PTAL.

In D87047#2252982 , @davidxl wrote:

> For x86 target, should it be turned on when -fprofile-use= option is 
> specified unless -fno-split-machine-function is specified?

I don't think we are there yet to turn it on for x86 profile builds. There is 
at least one unresolved issue with DebugInfo (D85085 
); once we have tested this some more 
internally we can revisit and make it the default.

> Also is it worth to give a warning if the option is specified but PGO is not 
> on?

Added a warning if no profile is specified but this option is turned on.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87047

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

[PATCH] D87047: [clang] Add command line options for the Machine Function Splitter.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish updated this revision to Diff 289805.
snehasish added a comment.

Add warning when option is enabled without profile.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87047

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/split-machine-functions.c
  clang/test/Driver/fsplit-machine-functions.c

Index: clang/test/Driver/fsplit-machine-functions.c
===
--- /dev/null
+++ clang/test/Driver/fsplit-machine-functions.c
@@ -0,0 +1,4 @@
+// RUN: %clang -### -fsplit-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
+// RUN: %clang -### -fsplit-machine-functions -fno-split-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-NOOPT %s
+// CHECK-OPT: "-fsplit-machine-functions"
+// CHECK-NOOPT-NOT: "-fsplit-machine-functions"
Index: clang/test/CodeGen/split-machine-functions.c
===
--- /dev/null
+++ clang/test/CodeGen/split-machine-functions.c
@@ -0,0 +1,34 @@
+// REQUIRES: x86-registered-target
+
+// RUN: echo "foo"> %t.proftext
+// RUN: echo "# Func Hash:"   >> %t.proftext
+// RUN: echo "11262309905">> %t.proftext
+// RUN: echo "# Num Counters:">> %t.proftext
+// RUN: echo "2"  >> %t.proftext
+// RUN: echo "# Counter Values:"  >> %t.proftext
+// RUN: echo "100">> %t.proftext
+// RUN: echo "0"  >> %t.proftext
+// RUN: llvm-profdata merge -o %t.profdata %t.proftext
+// RUN: %clang_cc1 -triple x86_64 -O3 -S -fprofile-instrument-use-path=%t.profdata -fsplit-machine-functions -o - < %s | FileCheck %s
+
+__attribute__((noinline)) int foo(int argc) {
+  if (argc % 2 == 0) {
+exit(argc);
+  } else {
+return argc + 1;
+  }
+}
+
+int main(int argc, char *argv[]) {
+  int total = 0;
+  for (int i = 0; i < 100; ++i) {
+total += foo(argc);
+  }
+  printf("%d\n", total);
+}
+
+// CHECK: .section .text.hot.,"ax",@progbits
+// CHECK: foo:
+// CHECK: section .text.unlikely.foo,"ax",@progbits
+// CHECK: foo.cold:
+// CHECK: callq exit@PLT
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -998,6 +998,8 @@
   Opts.UniqueInternalLinkageNames =
   Args.hasArg(OPT_funique_internal_linkage_names);
 
+  Opts.SplitMachineFunctions = Args.hasArg(OPT_fsplit_machine_functions);
+
   Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
 
   Opts.NoUseJumpTables = Args.hasArg(OPT_fno_jump_tables);
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4255,6 +4255,8 @@
 options::OPT_fno_unique_section_names,
 options::OPT_funique_basic_block_section_names,
 options::OPT_fno_unique_basic_block_section_names,
+options::OPT_fsplit_machine_functions,
+options::OPT_fno_split_machine_functions,
 options::OPT_mrestrict_it,
 options::OPT_mno_restrict_it,
 options::OPT_mstackrealign,
@@ -4905,6 +4907,10 @@
options::OPT_fno_unique_basic_block_section_names, false))
 CmdArgs.push_back("-funique-basic-block-section-names");
 
+  if (Args.hasFlag(options::OPT_fsplit_machine_functions,
+   options::OPT_fno_split_machine_functions, false))
+CmdArgs.push_back("-fsplit-machine-functions");
+
   Args.AddLastArg(CmdArgs, options::OPT_finstrument_functions,
   options::OPT_finstrument_functions_after_inlining,
   options::OPT_finstrument_function_entry_bare);
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -514,6 +514,13 @@
   Options.BBSectionsFuncListBuf = std::move(*MBOrErr);
   }
 
+  if (CodeGenOpts.SplitMachineFunctions) {
+if (CodeGenOpts.getProfileUse() != CodeGenOptions::ProfileNone)
+  Options.EnableMachineFunctionSplitter = true;
+else
+  Diags.Report(diag::warn_fe_machine_function_splitter_missing_profile);
+  }
+
   Options.FunctionSections = CodeGenOpts.FunctionSections;
   Options.DataSections = CodeGenOpts.DataSections;
   Options.UniqueSectionNames = CodeGenOpts.UniqueSectionNames;
Index: clang/include/clang/Driver/Options.td
===
--- 

[PATCH] D87047: [clang] Add command line options for the Machine Function Splitter.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish created this revision.
snehasish added reviewers: tmsriram, davidxl.
Herald added subscribers: cfe-commits, dang.
Herald added a project: clang.
snehasish requested review of this revision.

This patch adds a command line flag for the machine function splitter
(added in rG94faadaca4e1 
). The 
command line reference has been updated
with the new flag and tests added to check the pass is run (requires
profile data) and that the driver has registered the correct option.

-fsplit-machine-functions
Split machine functions using profile information (x86-elf only)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D87047

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/split-machine-functions.c
  clang/test/Driver/fsplit-machine-functions.c

Index: clang/test/Driver/fsplit-machine-functions.c
===
--- /dev/null
+++ clang/test/Driver/fsplit-machine-functions.c
@@ -0,0 +1,4 @@
+// RUN: %clang -### -fsplit-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
+// RUN: %clang -### -fsplit-machine-functions -fno-split-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-NOOPT %s
+// CHECK-OPT: "-fsplit-machine-functions"
+// CHECK-NOOPT-NOT: "-fsplit-machine-functions"
Index: clang/test/CodeGen/split-machine-functions.c
===
--- /dev/null
+++ clang/test/CodeGen/split-machine-functions.c
@@ -0,0 +1,34 @@
+// REQUIRES: x86-registered-target
+
+// RUN: echo "foo"> %t.proftext
+// RUN: echo "# Func Hash:"   >> %t.proftext
+// RUN: echo "11262309905">> %t.proftext
+// RUN: echo "# Num Counters:">> %t.proftext
+// RUN: echo "2"  >> %t.proftext
+// RUN: echo "# Counter Values:"  >> %t.proftext
+// RUN: echo "100">> %t.proftext
+// RUN: echo "0"  >> %t.proftext
+// RUN: llvm-profdata merge -o %t.profdata %t.proftext
+// RUN: %clang_cc1 -triple x86_64 -O3 -S -fprofile-instrument-use-path=%t.profdata -fsplit-machine-functions -o - < %s | FileCheck %s
+
+__attribute__((noinline)) int foo(int argc) {
+  if (argc % 2 == 0) {
+exit(argc);
+  } else {
+return argc + 1;
+  }
+}
+
+int main(int argc, char *argv[]) {
+  int total = 0;
+  for (int i = 0; i < 100; ++i) {
+total += foo(argc);
+  }
+  printf("%d\n", total);
+}
+
+// CHECK: .section .text.hot.,"ax",@progbits
+// CHECK: foo:
+// CHECK: section .text.unlikely.foo,"ax",@progbits
+// CHECK: foo.cold:
+// CHECK: callq exit@PLT
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -998,6 +998,8 @@
   Opts.UniqueInternalLinkageNames =
   Args.hasArg(OPT_funique_internal_linkage_names);
 
+  Opts.SplitMachineFunctions = Args.hasArg(OPT_fsplit_machine_functions);
+
   Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
 
   Opts.NoUseJumpTables = Args.hasArg(OPT_fno_jump_tables);
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4255,6 +4255,8 @@
 options::OPT_fno_unique_section_names,
 options::OPT_funique_basic_block_section_names,
 options::OPT_fno_unique_basic_block_section_names,
+options::OPT_fsplit_machine_functions,
+options::OPT_fno_split_machine_functions,
 options::OPT_mrestrict_it,
 options::OPT_mno_restrict_it,
 options::OPT_mstackrealign,
@@ -4905,6 +4907,10 @@
options::OPT_fno_unique_basic_block_section_names, false))
 CmdArgs.push_back("-funique-basic-block-section-names");
 
+  if (Args.hasFlag(options::OPT_fsplit_machine_functions,
+   options::OPT_fno_split_machine_functions, false))
+CmdArgs.push_back("-fsplit-machine-functions");
+
   Args.AddLastArg(CmdArgs, options::OPT_finstrument_functions,
   options::OPT_finstrument_functions_after_inlining,
   options::OPT_finstrument_function_entry_bare);
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -519,6 +519,7 @@
   Options.UniqueSectionNames = CodeGenOpts.UniqueSectionNames;
   Options.UniqueBasicBlockSectionNames =
   CodeGenOpts.UniqueBasicBlockSectionNames;
+  Options.EnableMachineFunctionSplitter = CodeGenOpts.SplitMachineFunctions;
   Options.TLSSize = 

[PATCH] D85408: Let -basic-block-sections=labels emit basicblock metadata in a new .bb_addr_map section, instead of emitting special unary-encoded symbols.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish accepted this revision.
snehasish added a comment.
This revision is now accepted and ready to land.

LGTM. Please wait a bit for additional comments from others.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85408

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

[PATCH] D85408: Let -basic-block-sections=labels emit basicblock metadata in a new .bb_addr_map section, instead of emitting special unary-encoded symbols.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish added inline comments.



Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1030
+/// recursive return edges vs. indirect branches. The format of the metadata is
+/// described as follows: 1st bit (LSB): set if this is a return block (return
+/// or tail call). 2nd bit: set if this is a block ending with a tail call. 3rd

Did you intend to change the formatting of the comment? I think the list items 
were on separate lines.



Comment at: llvm/lib/CodeGen/MachineBasicBlock.cpp:58
 
+unsigned MachineBasicBlock::getBBAddrMapMetadata() const {
+  const TargetInstrInfo *TII = getParent()->getSubtarget().getInstrInfo();

rahmanl wrote:
> snehasish wrote:
> > I think this method only relies on already public methods of 
> > MachineBasicBlock. It would be cleaner to move this from here to a static 
> > helper function in AsmPrinter.cpp. This way we don't add yet another method 
> > to the MachineBasicBlock class and keeps the relevant metadata generation 
> > logic close to where it is used.
> > 
> > Feel free to ignore if you plan on calling this method elsewhere apart from 
> > AsmPrinter.cpp but I can't think of a reason to do so.
> Thanks for the suggestion. It makes the code much more readable since I just 
> place that function above emitBBAddrMapSection.
> I can even make it a lambda function within emitBBAddrMapSection. WDYT?
I have a slight preference for the static method vs inline lambda since the 
comment describing the metadata can be placed above the static method.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85408

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

[PATCH] D85408: Let -basic-block-sections=labels emit basicblock metadata in a new .bb_addr_map section, instead of emitting special unary-encoded symbols.

[Snehasish Kumar via Phabricator via cfe-commits]

snehasish added inline comments.



Comment at: clang/docs/UsersManual.rst:1706
+  its own unique section. With the "labels" value, normal sections are emitted,
+  but a ".bb_addr_map" section is emitted which would include address offsets
+  for each basic block in the program, relative to the function address.

nit: s/would include/includes/



Comment at: clang/docs/UsersManual.rst:1707
+  but a ".bb_addr_map" section is emitted which would include address offsets
+  for each basic block in the program, relative to the function address.
 

nit: s/relative to the/relative to the parent/



Comment at: llvm/lib/CodeGen/MachineBasicBlock.cpp:58
 
+unsigned MachineBasicBlock::getBBAddrMapMetadata() const {
+  const TargetInstrInfo *TII = getParent()->getSubtarget().getInstrInfo();

I think this method only relies on already public methods of MachineBasicBlock. 
It would be cleaner to move this from here to a static helper function in 
AsmPrinter.cpp. This way we don't add yet another method to the 
MachineBasicBlock class and keeps the relevant metadata generation logic close 
to where it is used.

Feel free to ignore if you plan on calling this method elsewhere apart from 
AsmPrinter.cpp but I can't think of a reason to do so.



Comment at: llvm/test/CodeGen/X86/basic-block-sections-labels.ll:5-9
   %2 = alloca i8, align 1
   %3 = zext i1 %0 to i8
   store i8 %3, i8* %2, align 1
   %4 = load i8, i8* %2, align 1
   %5 = trunc i8 %4 to i1

I think this can be removed and the branch on L10 can directly use the param 
like so:

```
define void @_Z3bazb(i1 zeroext %0) personality i32 (...)* 
@__gxx_personality_v0 {
  br i1 %0, label %6, label %11
  
}
```

You will need to adjust the respective virtual register names since %2-%5 is no 
longer used.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85408

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