[PATCH] D126586: [InstrProf] Single byte counters in coverage

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

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



Comment at: clang/lib/CodeGen/CodeGenPGO.cpp:1080-1081
+void CodeGenPGO::setProfileVersion(llvm::Module ) {
+  if (CGM.getCodeGenOpts().hasProfileClangInstr() &&
+  llvm::EnableSingleByteCoverage) {
+const StringRef VarName(INSTR_PROF_QUOTE(INSTR_PROF_RAW_VERSION_VAR));

zequanwu wrote:
> I thinkit's better to emit the profile version global variable in 
> `CoverageMappingModuleGen::emit` so this check 
> `CGM.getCodeGenOpts().hasProfileClangInstr()` is not necessary. 
> 
> Also we should always emit the profile version global variable so that the 
> runtime/backend knows if single byte coverage is enabled or not by looking at 
> the version.
`CoverageMappingModuleGen` is only invoked when `-fcoverage-mapping` flag is 
provided, and single byte counters are emitted when `-fprofile-instr-generate` 
flag is provided. We provide both flags for coverage purposes, but if we move 
this global to `CoverageMappingModuleGen::emit`, single byte counters won't 
work if `-fcoverage-mapping` is not provided. I don't know whether there will 
be such use cases, but for this reason it might not be a good idea to put that 
into `CoverageMappingModuleGen`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126586

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

[PATCH] D126586: [InstrProf] Single byte counters in coverage

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem updated this revision to Diff 42.
gulfem added a comment.

1. Use || to merge counters that refer to the same regions
2. Use || to merge counters from raw profiles


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126586

Files:
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/lib/CodeGen/CGExprComplex.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenPGO.cpp
  clang/lib/CodeGen/CodeGenPGO.h
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/test/CoverageMapping/single-byte-counters.cpp
  llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
  llvm/include/llvm/ProfileData/InstrProf.h
  llvm/include/llvm/ProfileData/InstrProfWriter.h
  llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
  llvm/lib/ProfileData/InstrProf.cpp
  llvm/lib/ProfileData/InstrProfWriter.cpp

Index: llvm/lib/ProfileData/InstrProfWriter.cpp
===
--- llvm/lib/ProfileData/InstrProfWriter.cpp
+++ llvm/lib/ProfileData/InstrProfWriter.cpp
@@ -253,7 +253,7 @@
   Dest.scale(Weight, 1, MapWarn);
   } else {
 // We're updating a function we've seen before.
-Dest.merge(I, Weight, MapWarn);
+Dest.merge(I, Weight, MapWarn, hasSingleByteCoverage());
   }
 
   Dest.sortValueData();
Index: llvm/lib/ProfileData/InstrProf.cpp
===
--- llvm/lib/ProfileData/InstrProf.cpp
+++ llvm/lib/ProfileData/InstrProf.cpp
@@ -693,7 +693,8 @@
 }
 
 void InstrProfRecord::merge(InstrProfRecord , uint64_t Weight,
-function_ref Warn) {
+function_ref Warn,
+bool HasSingleByteCoverage) {
   // If the number of counters doesn't match we either have bad data
   // or a hash collision.
   if (Counts.size() != Other.Counts.size()) {
@@ -721,15 +722,23 @@
 
   for (size_t I = 0, E = Other.Counts.size(); I < E; ++I) {
 bool Overflowed;
-uint64_t Value =
-SaturatingMultiplyAdd(Other.Counts[I], Weight, Counts[I], );
-if (Value > getInstrMaxCountValue()) {
-  Value = getInstrMaxCountValue();
-  Overflowed = true;
+uint64_t Value;
+// When a profile has single byte coverage, use || to merge counters.
+if (HasSingleByteCoverage)
+  Value = Other.Counts[I] || Counts[I];
+else {
+  Value = SaturatingMultiplyAdd(Other.Counts[I], Weight, Counts[I],
+);
+
+  if (Value > getInstrMaxCountValue()) {
+Value = getInstrMaxCountValue();
+Overflowed = true;
+  }
+
+  if (Overflowed)
+Warn(instrprof_error::counter_overflow);
 }
 Counts[I] = Value;
-if (Overflowed)
-  Warn(instrprof_error::counter_overflow);
   }
 
   for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; ++Kind)
Index: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
===
--- llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
+++ llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
@@ -283,7 +283,8 @@
   consumeError(std::move(E));
   return Error::success();
 }
-Function.pushRegion(Region, *ExecutionCount, *AltExecutionCount);
+Function.pushRegion(Region, *ExecutionCount, *AltExecutionCount,
+ProfileReader.hasSingleByteCoverage());
   }
 
   // Don't create records for (filenames, function) pairs we've already seen.
@@ -675,8 +676,14 @@
   // value for that area.
   // We add counts of the regions of the same kind as the active region
   // to handle the both situations.
-  if (I->Kind == Active->Kind)
-Active->ExecutionCount += I->ExecutionCount;
+  if (I->Kind == Active->Kind) {
+assert(I->HasSingleByteCoverage == Active->HasSingleByteCoverage &&
+   "Regions are generated in different coverage modes");
+if (I->HasSingleByteCoverage)
+  Active->ExecutionCount = Active->ExecutionCount || I->ExecutionCount;
+else
+  Active->ExecutionCount += I->ExecutionCount;
+  }
 }
 return Regions.drop_back(std::distance(++Active, End));
   }
Index: llvm/include/llvm/ProfileData/InstrProfWriter.h
===
--- llvm/include/llvm/ProfileData/InstrProfWriter.h
+++ llvm/include/llvm/ProfileData/InstrProfWriter.h
@@ -169,6 +169,10 @@
 
   InstrProfKind getProfileKind() const { return ProfileKind; }
 
+  bool hasSingleByteCoverage() const {
+return static_cast(ProfileKind & InstrProfKind::SingleByteCoverage);
+  }
+
   // Internal interface for testing purpose only.
   void setValueProfDataEndianness(support::endianness Endianness);
   void setOutputSparse(bool Sparse);
Index: 

[PATCH] D126586: [InstrProf] Single byte counters in coverage

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem updated this revision to Diff 553992.
gulfem added a comment.

Fixed the issue that causes a crash in InstCombinePHI.cpp.
Phi instructions need to be inserted at the beginning of basic blocks,
and profile increments need to inserted after phis for conditional operators.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126586

Files:
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/lib/CodeGen/CGExprComplex.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenPGO.cpp
  clang/lib/CodeGen/CodeGenPGO.h
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/test/CoverageMapping/single-byte-counters.cpp

Index: clang/test/CoverageMapping/single-byte-counters.cpp
===
--- /dev/null
+++ clang/test/CoverageMapping/single-byte-counters.cpp
@@ -0,0 +1,169 @@
+// RUN: %clang_cc1 -mllvm -emptyline-comment-coverage=false -mllvm -enable-single-byte-coverage=true -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name single-byte-counters.cpp %s | FileCheck %s
+
+// CHECK: testIf
+int testIf(int x) { // CHECK-NEXT: File 0, [[@LINE]]:19 -> [[@LINE+10]]:2 = #0
+// CHECK-NEXT: File 0, [[@LINE+5]]:7 -> [[@LINE+5]]:13 = #0
+// CHECK-NEXT: Gap,File 0, [[@LINE+4]]:14 -> [[@LINE+5]]:5 = #1
+// CHECK-NEXT: File 0, [[@LINE+4]]:5 -> [[@LINE+4]]:16 = #1
+// CHECK-NEXT: File 0, [[@LINE+5]]:3 -> [[@LINE+5]]:16 = #2
+  int result = 0;
+  if (x == 0)
+result = -1;
+
+  return result;
+}
+
+// CHECK-NEXT: testIfElse
+int testIfElse(int x) { // CHECK-NEXT: File 0, [[@LINE]]:23 -> [[@LINE+13]]:2 = #0
+// CHECK-NEXT: File 0, [[@LINE+7]]:7 -> [[@LINE+7]]:12 = #0
+// CHECK-NEXT: Gap,File 0, [[@LINE+6]]:13 -> [[@LINE+7]]:5 = #1
+// CHECK-NEXT: File 0, [[@LINE+6]]:5 -> [[@LINE+6]]:15 = #1
+// CHECK-NEXT: Gap,File 0, [[@LINE+5]]:16 -> [[@LINE+7]]:5 = #2
+// CHECK-NEXT: File 0, [[@LINE+6]]:5 -> [[@LINE+6]]:19 = #2
+// CHECK-NEXT: File 0, [[@LINE+6]]:3 -> [[@LINE+6]]:16 = #3
+  int result = 0;
+  if (x < 0)
+result = 0;
+  else
+result = x * x;
+  return result;
+}
+
+// CHECK-NEXT: testIfElseReturn
+int testIfElseReturn(int x) { // CHECK-NEXT: File 0, [[@LINE]]:29 -> [[@LINE+14]]:2 = #0
+  // CHECK-NEXT: File 0, [[@LINE+8]]:7 -> [[@LINE+8]]:12 = #0
+  // CHECK-NEXT: Gap,File 0, [[@LINE+7]]:13 -> [[@LINE+8]]:5 = #1
+  // CHECK-NEXT: File 0, [[@LINE+7]]:5 -> [[@LINE+7]]:19 = #1
+  // CHECK-NEXT: Gap,File 0, [[@LINE+6]]:20 -> [[@LINE+8]]:5 = #2
+  // CHECK-NEXT: File 0, [[@LINE+7]]:5 -> [[@LINE+7]]:13 = #2
+  // CHECK-NEXT: Gap,File 0, [[@LINE+6]]:14 -> [[@LINE+7]]:3 = #3
+  // CHECK-NEXT: File 0, [[@LINE+6]]:3 -> [[@LINE+6]]:16 = #3
+  int result = 0;
+  if (x > 0)
+result = x * x;
+  else
+return 0;
+  return result;
+}
+
+// CHECK-NEXT: testSwitch
+int testSwitch(int x) { // CHECK-NEXT: File 0, [[@LINE]]:23 -> [[@LINE+22]]:2 = #0
+// CHECK-NEXT: Gap,File 0, [[@LINE+9]]:14 -> [[@LINE+17]]:15 = 0
+// CHECK-NEXT: File 0, [[@LINE+9]]:3 -> [[@LINE+11]]:10 = #2
+// CHECK-NEXT: Gap,File 0, [[@LINE+10]]:11 -> [[@LINE+11]]:3 = 0
+// CHECK-NEXT: File 0, [[@LINE+10]]:3 -> [[@LINE+12]]:10 = #3
+// CHECK-NEXT: Gap,File 0, [[@LINE+11]]:11 -> [[@LINE+12]]:3 = 0
+// CHECK-NEXT: File 0, [[@LINE+11]]:3 -> [[@LINE+12]]:15 = #4
+// CHECK-NEXT: Gap,File 0, [[@LINE+12]]:4 -> [[@LINE+14]]:3 = #1
+// CHECK-NEXT: File 0, [[@LINE+13]]:3 -> [[@LINE+13]]:16 = #1
+  int result;
+  switch (x) {
+  case 1:
+result = 1;
+break;
+  case 2:
+result = 2;
+break;
+  default:
+result = 0;
+  }
+
+  return result;
+}
+
+// CHECK-NEXT: testWhile
+int testWhile() { // CHECK-NEXT: File 0, [[@LINE]]:17 -> [[@LINE+13]]:2 = #0
+  // CHECK-NEXT: File 0, [[@LINE+6]]:10 -> [[@LINE+6]]:16 = #1
+  // CHECK-NEXT: Gap,File 0, [[@LINE+5]]:17 -> [[@LINE+5]]:18 = #2
+  // CHECK-NEXT: File 0, [[@LINE+4]]:18 -> [[@LINE+7]]:4 = #2
+  // CHECK-NEXT: File 0, [[@LINE+8]]:3 -> [[@LINE+8]]:13 = #3
+  int i = 0;
+  int sum = 0;
+  while (i < 10) {
+sum += i;
+i++;
+  }
+
+  return sum;
+}
+
+// CHECK-NEXT: testContinue
+int testContinue() { // 

[PATCH] D126586: [InstrProf] Single byte counters in coverage

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem updated this revision to Diff 551687.
gulfem added a comment.

Remove hyphen between "single counter" for consistency


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126586

Files:
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/lib/CodeGen/CGExprComplex.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenPGO.cpp
  clang/lib/CodeGen/CodeGenPGO.h
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/test/CoverageMapping/single-byte-counters.cpp

Index: clang/test/CoverageMapping/single-byte-counters.cpp
===
--- /dev/null
+++ clang/test/CoverageMapping/single-byte-counters.cpp
@@ -0,0 +1,169 @@
+// RUN: %clang_cc1 -mllvm -emptyline-comment-coverage=false -mllvm -enable-single-byte-coverage=true -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name single-byte-counters.cpp %s | FileCheck %s
+
+// CHECK: testIf
+int testIf(int x) { // CHECK-NEXT: File 0, [[@LINE]]:19 -> [[@LINE+10]]:2 = #0
+// CHECK-NEXT: File 0, [[@LINE+5]]:7 -> [[@LINE+5]]:13 = #0
+// CHECK-NEXT: Gap,File 0, [[@LINE+4]]:14 -> [[@LINE+5]]:5 = #1
+// CHECK-NEXT: File 0, [[@LINE+4]]:5 -> [[@LINE+4]]:16 = #1
+// CHECK-NEXT: File 0, [[@LINE+5]]:3 -> [[@LINE+5]]:16 = #2
+  int result = 0;
+  if (x == 0)
+result = -1;
+
+  return result;
+}
+
+// CHECK-NEXT: testIfElse
+int testIfElse(int x) { // CHECK-NEXT: File 0, [[@LINE]]:23 -> [[@LINE+13]]:2 = #0
+// CHECK-NEXT: File 0, [[@LINE+7]]:7 -> [[@LINE+7]]:12 = #0
+// CHECK-NEXT: Gap,File 0, [[@LINE+6]]:13 -> [[@LINE+7]]:5 = #1
+// CHECK-NEXT: File 0, [[@LINE+6]]:5 -> [[@LINE+6]]:15 = #1
+// CHECK-NEXT: Gap,File 0, [[@LINE+5]]:16 -> [[@LINE+7]]:5 = #2
+// CHECK-NEXT: File 0, [[@LINE+6]]:5 -> [[@LINE+6]]:19 = #2
+// CHECK-NEXT: File 0, [[@LINE+6]]:3 -> [[@LINE+6]]:16 = #3
+  int result = 0;
+  if (x < 0)
+result = 0;
+  else
+result = x * x;
+  return result;
+}
+
+// CHECK-NEXT: testIfElseReturn
+int testIfElseReturn(int x) { // CHECK-NEXT: File 0, [[@LINE]]:29 -> [[@LINE+14]]:2 = #0
+  // CHECK-NEXT: File 0, [[@LINE+8]]:7 -> [[@LINE+8]]:12 = #0
+  // CHECK-NEXT: Gap,File 0, [[@LINE+7]]:13 -> [[@LINE+8]]:5 = #1
+  // CHECK-NEXT: File 0, [[@LINE+7]]:5 -> [[@LINE+7]]:19 = #1
+  // CHECK-NEXT: Gap,File 0, [[@LINE+6]]:20 -> [[@LINE+8]]:5 = #2
+  // CHECK-NEXT: File 0, [[@LINE+7]]:5 -> [[@LINE+7]]:13 = #2
+  // CHECK-NEXT: Gap,File 0, [[@LINE+6]]:14 -> [[@LINE+7]]:3 = #3
+  // CHECK-NEXT: File 0, [[@LINE+6]]:3 -> [[@LINE+6]]:16 = #3
+  int result = 0;
+  if (x > 0)
+result = x * x;
+  else
+return 0;
+  return result;
+}
+
+// CHECK-NEXT: testSwitch
+int testSwitch(int x) { // CHECK-NEXT: File 0, [[@LINE]]:23 -> [[@LINE+22]]:2 = #0
+// CHECK-NEXT: Gap,File 0, [[@LINE+9]]:14 -> [[@LINE+17]]:15 = 0
+// CHECK-NEXT: File 0, [[@LINE+9]]:3 -> [[@LINE+11]]:10 = #2
+// CHECK-NEXT: Gap,File 0, [[@LINE+10]]:11 -> [[@LINE+11]]:3 = 0
+// CHECK-NEXT: File 0, [[@LINE+10]]:3 -> [[@LINE+12]]:10 = #3
+// CHECK-NEXT: Gap,File 0, [[@LINE+11]]:11 -> [[@LINE+12]]:3 = 0
+// CHECK-NEXT: File 0, [[@LINE+11]]:3 -> [[@LINE+12]]:15 = #4
+// CHECK-NEXT: Gap,File 0, [[@LINE+12]]:4 -> [[@LINE+14]]:3 = #1
+// CHECK-NEXT: File 0, [[@LINE+13]]:3 -> [[@LINE+13]]:16 = #1
+  int result;
+  switch (x) {
+  case 1:
+result = 1;
+break;
+  case 2:
+result = 2;
+break;
+  default:
+result = 0;
+  }
+
+  return result;
+}
+
+// CHECK-NEXT: testWhile
+int testWhile() { // CHECK-NEXT: File 0, [[@LINE]]:17 -> [[@LINE+13]]:2 = #0
+  // CHECK-NEXT: File 0, [[@LINE+6]]:10 -> [[@LINE+6]]:16 = #1
+  // CHECK-NEXT: Gap,File 0, [[@LINE+5]]:17 -> [[@LINE+5]]:18 = #2
+  // CHECK-NEXT: File 0, [[@LINE+4]]:18 -> [[@LINE+7]]:4 = #2
+  // CHECK-NEXT: File 0, [[@LINE+8]]:3 -> [[@LINE+8]]:13 = #3
+  int i = 0;
+  int sum = 0;
+  while (i < 10) {
+sum += i;
+i++;
+  }
+
+  return sum;
+}
+
+// CHECK-NEXT: testContinue
+int testContinue() { // CHECK-NEXT: File 0, [[@LINE]]:20 -> [[@LINE+21]]:2 = #0
+ // CHECK-NEXT: File 0, [[@LINE+12]]:10 -> [[@LINE+12]]:16 = #1
+ 

[PATCH] D126586: [InstrProf] Single byte counters in coverage

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem updated this revision to Diff 551686.
gulfem added a comment.

Ensure the correct traversal order for loops in CodeGenPGO
to fix the profile test failures (Profile/cxx-hash-v2.cpp
and Profile/cxx-rangefor.cpp)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126586

Files:
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/lib/CodeGen/CGExprComplex.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenPGO.cpp
  clang/lib/CodeGen/CodeGenPGO.h
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/test/CoverageMapping/single-byte-counters.cpp

Index: clang/test/CoverageMapping/single-byte-counters.cpp
===
--- /dev/null
+++ clang/test/CoverageMapping/single-byte-counters.cpp
@@ -0,0 +1,169 @@
+// RUN: %clang_cc1 -mllvm -emptyline-comment-coverage=false -mllvm -enable-single-byte-coverage=true -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name single-byte-counters.cpp %s | FileCheck %s
+
+// CHECK: testIf
+int testIf(int x) { // CHECK-NEXT: File 0, [[@LINE]]:19 -> [[@LINE+10]]:2 = #0
+// CHECK-NEXT: File 0, [[@LINE+5]]:7 -> [[@LINE+5]]:13 = #0
+// CHECK-NEXT: Gap,File 0, [[@LINE+4]]:14 -> [[@LINE+5]]:5 = #1
+// CHECK-NEXT: File 0, [[@LINE+4]]:5 -> [[@LINE+4]]:16 = #1
+// CHECK-NEXT: File 0, [[@LINE+5]]:3 -> [[@LINE+5]]:16 = #2
+  int result = 0;
+  if (x == 0)
+result = -1;
+
+  return result;
+}
+
+// CHECK-NEXT: testIfElse
+int testIfElse(int x) { // CHECK-NEXT: File 0, [[@LINE]]:23 -> [[@LINE+13]]:2 = #0
+// CHECK-NEXT: File 0, [[@LINE+7]]:7 -> [[@LINE+7]]:12 = #0
+// CHECK-NEXT: Gap,File 0, [[@LINE+6]]:13 -> [[@LINE+7]]:5 = #1
+// CHECK-NEXT: File 0, [[@LINE+6]]:5 -> [[@LINE+6]]:15 = #1
+// CHECK-NEXT: Gap,File 0, [[@LINE+5]]:16 -> [[@LINE+7]]:5 = #2
+// CHECK-NEXT: File 0, [[@LINE+6]]:5 -> [[@LINE+6]]:19 = #2
+// CHECK-NEXT: File 0, [[@LINE+6]]:3 -> [[@LINE+6]]:16 = #3
+  int result = 0;
+  if (x < 0)
+result = 0;
+  else
+result = x * x;
+  return result;
+}
+
+// CHECK-NEXT: testIfElseReturn
+int testIfElseReturn(int x) { // CHECK-NEXT: File 0, [[@LINE]]:29 -> [[@LINE+14]]:2 = #0
+  // CHECK-NEXT: File 0, [[@LINE+8]]:7 -> [[@LINE+8]]:12 = #0
+  // CHECK-NEXT: Gap,File 0, [[@LINE+7]]:13 -> [[@LINE+8]]:5 = #1
+  // CHECK-NEXT: File 0, [[@LINE+7]]:5 -> [[@LINE+7]]:19 = #1
+  // CHECK-NEXT: Gap,File 0, [[@LINE+6]]:20 -> [[@LINE+8]]:5 = #2
+  // CHECK-NEXT: File 0, [[@LINE+7]]:5 -> [[@LINE+7]]:13 = #2
+  // CHECK-NEXT: Gap,File 0, [[@LINE+6]]:14 -> [[@LINE+7]]:3 = #3
+  // CHECK-NEXT: File 0, [[@LINE+6]]:3 -> [[@LINE+6]]:16 = #3
+  int result = 0;
+  if (x > 0)
+result = x * x;
+  else
+return 0;
+  return result;
+}
+
+// CHECK-NEXT: testSwitch
+int testSwitch(int x) { // CHECK-NEXT: File 0, [[@LINE]]:23 -> [[@LINE+22]]:2 = #0
+// CHECK-NEXT: Gap,File 0, [[@LINE+9]]:14 -> [[@LINE+17]]:15 = 0
+// CHECK-NEXT: File 0, [[@LINE+9]]:3 -> [[@LINE+11]]:10 = #2
+// CHECK-NEXT: Gap,File 0, [[@LINE+10]]:11 -> [[@LINE+11]]:3 = 0
+// CHECK-NEXT: File 0, [[@LINE+10]]:3 -> [[@LINE+12]]:10 = #3
+// CHECK-NEXT: Gap,File 0, [[@LINE+11]]:11 -> [[@LINE+12]]:3 = 0
+// CHECK-NEXT: File 0, [[@LINE+11]]:3 -> [[@LINE+12]]:15 = #4
+// CHECK-NEXT: Gap,File 0, [[@LINE+12]]:4 -> [[@LINE+14]]:3 = #1
+// CHECK-NEXT: File 0, [[@LINE+13]]:3 -> [[@LINE+13]]:16 = #1
+  int result;
+  switch (x) {
+  case 1:
+result = 1;
+break;
+  case 2:
+result = 2;
+break;
+  default:
+result = 0;
+  }
+
+  return result;
+}
+
+// CHECK-NEXT: testWhile
+int testWhile() { // CHECK-NEXT: File 0, [[@LINE]]:17 -> [[@LINE+13]]:2 = #0
+  // CHECK-NEXT: File 0, [[@LINE+6]]:10 -> [[@LINE+6]]:16 = #1
+  // CHECK-NEXT: Gap,File 0, [[@LINE+5]]:17 -> [[@LINE+5]]:18 = #2
+  // CHECK-NEXT: File 0, [[@LINE+4]]:18 -> [[@LINE+7]]:4 = #2
+  // CHECK-NEXT: File 0, [[@LINE+8]]:3 -> [[@LINE+8]]:13 = #3
+  int i = 0;
+  int sum = 0;
+  while (i < 10) {
+sum += i;
+i++;
+  }
+
+  return sum;
+}
+
+// CHECK-NEXT: testContinue
+int testContinue() { // CHECK-NEXT: File 0, [[@LINE]]:20 -> [[@LINE+21]]:2 = #0
+   

[PATCH] D126586: [InstrProf][WIP] Implement boolean counters in coverage

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem updated this revision to Diff 550163.
gulfem added a comment.

- Extend the prototype by implementing the rest of the control-flow statements
- Rename the prototype to single byte counters to be consistent with the single 
byte coverage used in PGO


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126586

Files:
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/lib/CodeGen/CGExprComplex.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenPGO.cpp
  clang/lib/CodeGen/CodeGenPGO.h
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/test/CoverageMapping/single-byte-counters.cpp

Index: clang/test/CoverageMapping/single-byte-counters.cpp
===
--- /dev/null
+++ clang/test/CoverageMapping/single-byte-counters.cpp
@@ -0,0 +1,169 @@
+// RUN: %clang_cc1 -mllvm -emptyline-comment-coverage=false -mllvm -enable-single-byte-coverage=true -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name single-byte-counters.cpp %s | FileCheck %s
+
+// CHECK: testIf
+int testIf(int x) { // CHECK-NEXT: File 0, [[@LINE]]:19 -> [[@LINE+10]]:2 = #0
+// CHECK-NEXT: File 0, [[@LINE+5]]:7 -> [[@LINE+5]]:13 = #0
+// CHECK-NEXT: Gap,File 0, [[@LINE+4]]:14 -> [[@LINE+5]]:5 = #2
+// CHECK-NEXT: File 0, [[@LINE+4]]:5 -> [[@LINE+4]]:16 = #2
+// CHECK-NEXT: File 0, [[@LINE+5]]:3 -> [[@LINE+5]]:16 = #1
+  int result = 0;
+  if (x == 0)
+result = -1;
+
+  return result;
+}
+
+// CHECK-NEXT: testIfElse
+int testIfElse(int x) { // CHECK-NEXT: File 0, [[@LINE]]:23 -> [[@LINE+13]]:2 = #0
+// CHECK-NEXT: File 0, [[@LINE+7]]:7 -> [[@LINE+7]]:12 = #0
+// CHECK-NEXT: Gap,File 0, [[@LINE+6]]:13 -> [[@LINE+7]]:5 = #2
+// CHECK-NEXT: File 0, [[@LINE+6]]:5 -> [[@LINE+6]]:15 = #2
+// CHECK-NEXT: Gap,File 0, [[@LINE+5]]:16 -> [[@LINE+7]]:5 = #3
+// CHECK-NEXT: File 0, [[@LINE+6]]:5 -> [[@LINE+6]]:19 = #3
+// CHECK-NEXT: File 0, [[@LINE+6]]:3 -> [[@LINE+6]]:16 = #1
+  int result = 0;
+  if (x < 0)
+result = 0;
+  else
+result = x * x;
+  return result;
+}
+
+// CHECK-NEXT: testIfElseReturn
+int testIfElseReturn(int x) { // CHECK-NEXT: File 0, [[@LINE]]:29 -> [[@LINE+14]]:2 = #0
+  // CHECK-NEXT: File 0, [[@LINE+8]]:7 -> [[@LINE+8]]:12 = #0
+  // CHECK-NEXT: Gap,File 0, [[@LINE+7]]:13 -> [[@LINE+8]]:5 = #2
+  // CHECK-NEXT: File 0, [[@LINE+7]]:5 -> [[@LINE+7]]:19 = #2
+  // CHECK-NEXT: Gap,File 0, [[@LINE+6]]:20 -> [[@LINE+8]]:5 = #3
+  // CHECK-NEXT: File 0, [[@LINE+7]]:5 -> [[@LINE+7]]:13 = #3
+  // CHECK-NEXT: Gap,File 0, [[@LINE+6]]:14 -> [[@LINE+7]]:3 = #1
+  // CHECK-NEXT: File 0, [[@LINE+6]]:3 -> [[@LINE+6]]:16 = #1
+  int result = 0;
+  if (x > 0)
+result = x * x;
+  else
+return 0;
+  return result;
+}
+
+// CHECK-NEXT: testSwitch
+int testSwitch(int x) { // CHECK-NEXT: File 0, [[@LINE]]:23 -> [[@LINE+22]]:2 = #0
+// CHECK-NEXT: Gap,File 0, [[@LINE+9]]:14 -> [[@LINE+17]]:15 = 0
+// CHECK-NEXT: File 0, [[@LINE+9]]:3 -> [[@LINE+11]]:10 = #2
+// CHECK-NEXT: Gap,File 0, [[@LINE+10]]:11 -> [[@LINE+11]]:3 = 0
+// CHECK-NEXT: File 0, [[@LINE+10]]:3 -> [[@LINE+12]]:10 = #3
+// CHECK-NEXT: Gap,File 0, [[@LINE+11]]:11 -> [[@LINE+12]]:3 = 0
+// CHECK-NEXT: File 0, [[@LINE+11]]:3 -> [[@LINE+12]]:15 = #4
+// CHECK-NEXT: Gap,File 0, [[@LINE+12]]:4 -> [[@LINE+14]]:3 = #1
+// CHECK-NEXT: File 0, [[@LINE+13]]:3 -> [[@LINE+13]]:16 = #1
+  int result;
+  switch (x) {
+  case 1:
+result = 1;
+break;
+  case 2:
+result = 2;
+break;
+  default:
+result = 0;
+  }
+
+  return result;
+}
+
+// CHECK-NEXT: testWhile
+int testWhile() { // CHECK-NEXT: File 0, [[@LINE]]:17 -> [[@LINE+13]]:2 = #0
+  // CHECK-NEXT: File 0, [[@LINE+6]]:10 -> [[@LINE+6]]:16 = #2
+  // CHECK-NEXT: Gap,File 0, [[@LINE+5]]:17 -> [[@LINE+5]]:18 = #3
+  // CHECK-NEXT: File 0, [[@LINE+4]]:18 -> [[@LINE+7]]:4 = #3
+  // CHECK-NEXT: File 0, [[@LINE+8]]:3 -> [[@LINE+8]]:13 = #1
+  int i = 0;
+  int sum = 0;
+  while (i < 10) {
+sum += i;
+i++;
+  }
+
+  return sum;
+}
+
+// CHECK-NEXT: testContinue
+int testContinue() { // CHECK-NEXT: File 0, [[@LINE]]:20 

[PATCH] D157944: [InstrProf] Single byte counters in coverage

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem created this revision.
Herald added subscribers: wlei, ellis, wenlei.
Herald added a project: All.
gulfem requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This patch inserts 1-byte counters instead of an 8-byte counters
into llvm profiles for source-based code coverage. The origial idea
was proposed as "block-cov" for PGO, and this patch repurposes that
idea for coverage: https://groups.google.com/g/llvm-dev/c/r03Z6JoN7d4

The current 8-byte counters mechanism add counters to minimal regions,
and infer the counters in the remaining regions via adding or
subtracting counters. For example, it infers the counter in the `if.else`
region by subtracting the counters between `if.entry` and `if.then` regions
in an if statement. Whenever there is a control-flow merge, it adds
the counters from all the incoming regions. However, we are not going to be
able to infer counters by subtracting two execution counts when using
single-byte counters. Therefore, this patch conservatively inserts additional
counters for the cases where we need to add or subtract counters.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D157944

Files:
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/lib/CodeGen/CGExprComplex.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenPGO.cpp
  clang/lib/CodeGen/CodeGenPGO.h
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/test/CoverageMapping/single-byte-counters.cpp

Index: clang/test/CoverageMapping/single-byte-counters.cpp
===
--- /dev/null
+++ clang/test/CoverageMapping/single-byte-counters.cpp
@@ -0,0 +1,169 @@
+// RUN: %clang_cc1 -mllvm -emptyline-comment-coverage=false -mllvm -enable-single-byte-coverage=true -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name single-byte-counters.cpp %s | FileCheck %s
+
+// CHECK: testIf
+int testIf(int x) { // CHECK-NEXT: File 0, [[@LINE]]:19 -> [[@LINE+10]]:2 = #0
+// CHECK-NEXT: File 0, [[@LINE+5]]:7 -> [[@LINE+5]]:13 = #0
+// CHECK-NEXT: Gap,File 0, [[@LINE+4]]:14 -> [[@LINE+5]]:5 = #2
+// CHECK-NEXT: File 0, [[@LINE+4]]:5 -> [[@LINE+4]]:16 = #2
+// CHECK-NEXT: File 0, [[@LINE+5]]:3 -> [[@LINE+5]]:16 = #1
+  int result = 0;
+  if (x == 0)
+result = -1;
+
+  return result;
+}
+
+// CHECK-NEXT: testIfElse
+int testIfElse(int x) { // CHECK-NEXT: File 0, [[@LINE]]:23 -> [[@LINE+13]]:2 = #0
+// CHECK-NEXT: File 0, [[@LINE+7]]:7 -> [[@LINE+7]]:12 = #0
+// CHECK-NEXT: Gap,File 0, [[@LINE+6]]:13 -> [[@LINE+7]]:5 = #2
+// CHECK-NEXT: File 0, [[@LINE+6]]:5 -> [[@LINE+6]]:15 = #2
+// CHECK-NEXT: Gap,File 0, [[@LINE+5]]:16 -> [[@LINE+7]]:5 = #3
+// CHECK-NEXT: File 0, [[@LINE+6]]:5 -> [[@LINE+6]]:19 = #3
+// CHECK-NEXT: File 0, [[@LINE+6]]:3 -> [[@LINE+6]]:16 = #1
+  int result = 0;
+  if (x < 0)
+result = 0;
+  else
+result = x * x;
+  return result;
+}
+
+// CHECK-NEXT: testIfElseReturn
+int testIfElseReturn(int x) { // CHECK-NEXT: File 0, [[@LINE]]:29 -> [[@LINE+14]]:2 = #0
+  // CHECK-NEXT: File 0, [[@LINE+8]]:7 -> [[@LINE+8]]:12 = #0
+  // CHECK-NEXT: Gap,File 0, [[@LINE+7]]:13 -> [[@LINE+8]]:5 = #2
+  // CHECK-NEXT: File 0, [[@LINE+7]]:5 -> [[@LINE+7]]:19 = #2
+  // CHECK-NEXT: Gap,File 0, [[@LINE+6]]:20 -> [[@LINE+8]]:5 = #3
+  // CHECK-NEXT: File 0, [[@LINE+7]]:5 -> [[@LINE+7]]:13 = #3
+  // CHECK-NEXT: Gap,File 0, [[@LINE+6]]:14 -> [[@LINE+7]]:3 = #1
+  // CHECK-NEXT: File 0, [[@LINE+6]]:3 -> [[@LINE+6]]:16 = #1
+  int result = 0;
+  if (x > 0)
+result = x * x;
+  else
+return 0;
+  return result;
+}
+
+// CHECK-NEXT: testSwitch
+int testSwitch(int x) { // CHECK-NEXT: File 0, [[@LINE]]:23 -> [[@LINE+22]]:2 = #0
+// CHECK-NEXT: Gap,File 0, [[@LINE+9]]:14 -> [[@LINE+17]]:15 = 0
+// CHECK-NEXT: File 0, [[@LINE+9]]:3 -> [[@LINE+11]]:10 = #2
+// CHECK-NEXT: Gap,File 0, [[@LINE+10]]:11 -> [[@LINE+11]]:3 = 0
+// CHECK-NEXT: File 0, [[@LINE+10]]:3 -> [[@LINE+12]]:10 = #3
+// CHECK-NEXT: Gap,File 0, [[@LINE+11]]:11 -> [[@LINE+12]]:3 = 0
+// CHECK-NEXT: File 0, [[@LINE+11]]:3 -> [[@LINE+12]]:15 = #4
+// CHECK-NEXT: Gap,File 0, [[@LINE+12]]:4 -> [[@LINE+14]]:3 = #1
+// CHECK-NEXT: File 0, 

[PATCH] D127040: [InstrProf][WIP] Implement boolean counters in coverage

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem abandoned this revision.
gulfem added a comment.
Herald added a subscriber: wlei.

Created a duplicate review by mistake.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127040

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

[PATCH] D148757: [clang] Apply -fcoverage-prefix-map reverse order

[Gulfem Savrun Yeniceri 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 rGd7fa92126cc3: [clang] Apply -fcoverage-prefix-map reverse 
order (authored by gulfem).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148757

Files:
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Profile/coverage-prefix-map.c


Index: clang/test/Profile/coverage-prefix-map.c
===
--- clang/test/Profile/coverage-prefix-map.c
+++ clang/test/Profile/coverage-prefix-map.c
@@ -19,3 +19,13 @@
 
 // RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -emit-llvm 
-mllvm -enable-name-compression=false -main-file-name coverage-prefix-map.c 
%t/root/nested/coverage-prefix-map.c -fcoverage-compilation-dir=/custom 
-fcoverage-prefix-map=/custom=/nonsense -o - | FileCheck 
--check-prefix=COVERAGE-COMPILATION-DIR %s
 // COVERAGE-COMPILATION-DIR: @__llvm_coverage_mapping = {{.*"\\02.*}}nonsense
+
+// Test that last -fcoverage-prefix-map option 
(-fcoverage-prefix-map==newpath) is applied.
+// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -emit-llvm 
-mllvm -enable-name-compression=false -main-file-name coverage-prefix-map.c 
%t/root/nested/coverage-prefix-map.c -fcoverage-prefix-map=%/t/root=. 
-fcoverage-prefix-map==newpath -o - | FileCheck 
--check-prefix=COVERAGE-PREFIX-MAP-ORDER %s
+// COVERAGE-PREFIX-MAP-ORDER: @__llvm_coverage_mapping = 
{{.*"\\02.*newpath.*root.*nested.*coverage-prefix-map\.c}}
+
+// Test that last -fcoverage-prefix-map option 
(-fcoverage-prefix-map=%/t/root=.) is applied.
+// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -emit-llvm 
-mllvm -enable-name-compression=false -main-file-name coverage-prefix-map.c 
%t/root/nested/coverage-prefix-map.c -fcoverage-prefix-map==newpath 
-fcoverage-prefix-map=%/t/root=. -o - | FileCheck 
--check-prefix=COVERAGE-PREFIX-MAP-REORDER %s
+// COVERAGE-PREFIX-MAP-REORDER: @__llvm_coverage_mapping =
+// COVERAGE-PREFIX-MAP-REORDER-NOT: newpath
+// COVERAGE-PREFIX-MAP-REORDER-SAME: nested{{.*coverage-prefix-map\.c}}
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -1701,8 +1701,7 @@
 
   for (const auto  : Args.getAllArgValues(OPT_fcoverage_prefix_map_EQ)) {
 auto Split = StringRef(Arg).split('=');
-Opts.CoveragePrefixMap.insert(
-{std::string(Split.first), std::string(Split.second)});
+Opts.CoveragePrefixMap.emplace_back(Split.first, Split.second);
   }
 
   const llvm::Triple::ArchType DebugEntryValueArchs[] = {
Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -1648,8 +1648,13 @@
 std::string CoverageMappingModuleGen::normalizeFilename(StringRef Filename) {
   llvm::SmallString<256> Path(Filename);
   llvm::sys::path::remove_dots(Path, /*remove_dot_dot=*/true);
-  for (const auto  : CGM.getCodeGenOpts().CoveragePrefixMap) {
-if (llvm::sys::path::replace_path_prefix(Path, Entry.first, Entry.second))
+
+  /// Traverse coverage prefix map in reverse order because prefix replacements
+  /// are applied in reverse order starting from the last one when multiple
+  /// prefix replacement options are provided.
+  for (const auto &[From, To] :
+   llvm::reverse(CGM.getCodeGenOpts().CoveragePrefixMap)) {
+if (llvm::sys::path::replace_path_prefix(Path, From, To))
   break;
   }
   return Path.str().str();
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -3223,7 +3223,8 @@
 def fcoverage_prefix_map_EQ
   : Joined<["-"], "fcoverage-prefix-map=">, Group,
 Flags<[CC1Option]>,
-HelpText<"remap file source paths in coverage mapping">;
+MetaVarName<"=">,
+HelpText<"remap file source paths  to  in coverage mapping. If 
there are multiple options, prefix replacement is applied in reverse order 
starting from the last one">;
 def ffile_prefix_map_EQ
   : Joined<["-"], "ffile-prefix-map=">, Group,
 HelpText<"remap file source paths in debug info, predefined preprocessor "
Index: clang/include/clang/Basic/CodeGenOptions.h
===
--- clang/include/clang/Basic/CodeGenOptions.h
+++ clang/include/clang/Basic/CodeGenOptions.h
@@ -207,7 +207,10 @@
   std::string RecordCommandLine;
 
   llvm::SmallVector, 0> DebugPrefixMap;
-  std::map 

[PATCH] D148757: [clang] Apply -fcoverage-prefix-map reverse order

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem updated this revision to Diff 517400.
gulfem added a comment.

Extend the comment about the usage of CoveragePrefixMap.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148757

Files:
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Profile/coverage-prefix-map.c


Index: clang/test/Profile/coverage-prefix-map.c
===
--- clang/test/Profile/coverage-prefix-map.c
+++ clang/test/Profile/coverage-prefix-map.c
@@ -19,3 +19,13 @@
 
 // RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -emit-llvm 
-mllvm -enable-name-compression=false -main-file-name coverage-prefix-map.c 
%t/root/nested/coverage-prefix-map.c -fcoverage-compilation-dir=/custom 
-fcoverage-prefix-map=/custom=/nonsense -o - | FileCheck 
--check-prefix=COVERAGE-COMPILATION-DIR %s
 // COVERAGE-COMPILATION-DIR: @__llvm_coverage_mapping = {{.*"\\02.*}}nonsense
+
+// Test that last -fcoverage-prefix-map option 
(-fcoverage-prefix-map==newpath) is applied.
+// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -emit-llvm 
-mllvm -enable-name-compression=false -main-file-name coverage-prefix-map.c 
%t/root/nested/coverage-prefix-map.c -fcoverage-prefix-map=%/t/root=. 
-fcoverage-prefix-map==newpath -o - | FileCheck 
--check-prefix=COVERAGE-PREFIX-MAP-ORDER %s
+// COVERAGE-PREFIX-MAP-ORDER: @__llvm_coverage_mapping = 
{{.*"\\02.*newpath.*root.*nested.*coverage-prefix-map\.c}}
+
+// Test that last -fcoverage-prefix-map option 
(-fcoverage-prefix-map=%/t/root=.) is applied.
+// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -emit-llvm 
-mllvm -enable-name-compression=false -main-file-name coverage-prefix-map.c 
%t/root/nested/coverage-prefix-map.c -fcoverage-prefix-map==newpath 
-fcoverage-prefix-map=%/t/root=. -o - | FileCheck 
--check-prefix=COVERAGE-PREFIX-MAP-REORDER %s
+// COVERAGE-PREFIX-MAP-REORDER: @__llvm_coverage_mapping =
+// COVERAGE-PREFIX-MAP-REORDER-NOT: newpath
+// COVERAGE-PREFIX-MAP-REORDER-SAME: nested{{.*coverage-prefix-map\.c}}
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -1701,8 +1701,7 @@
 
   for (const auto  : Args.getAllArgValues(OPT_fcoverage_prefix_map_EQ)) {
 auto Split = StringRef(Arg).split('=');
-Opts.CoveragePrefixMap.insert(
-{std::string(Split.first), std::string(Split.second)});
+Opts.CoveragePrefixMap.emplace_back(Split.first, Split.second);
   }
 
   const llvm::Triple::ArchType DebugEntryValueArchs[] = {
Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -1648,8 +1648,13 @@
 std::string CoverageMappingModuleGen::normalizeFilename(StringRef Filename) {
   llvm::SmallString<256> Path(Filename);
   llvm::sys::path::remove_dots(Path, /*remove_dot_dot=*/true);
-  for (const auto  : CGM.getCodeGenOpts().CoveragePrefixMap) {
-if (llvm::sys::path::replace_path_prefix(Path, Entry.first, Entry.second))
+
+  /// Traverse coverage prefix map in reverse order because prefix replacements
+  /// are applied in reverse order starting from the last one when multiple
+  /// prefix replacement options are provided.
+  for (const auto &[From, To] :
+   llvm::reverse(CGM.getCodeGenOpts().CoveragePrefixMap)) {
+if (llvm::sys::path::replace_path_prefix(Path, From, To))
   break;
   }
   return Path.str().str();
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -3223,7 +3223,8 @@
 def fcoverage_prefix_map_EQ
   : Joined<["-"], "fcoverage-prefix-map=">, Group,
 Flags<[CC1Option]>,
-HelpText<"remap file source paths in coverage mapping">;
+MetaVarName<"=">,
+HelpText<"remap file source paths  to  in coverage mapping. If 
there are multiple options, prefix replacement is applied in reverse order 
starting from the last one">;
 def ffile_prefix_map_EQ
   : Joined<["-"], "ffile-prefix-map=">, Group,
 HelpText<"remap file source paths in debug info, predefined preprocessor "
Index: clang/include/clang/Basic/CodeGenOptions.h
===
--- clang/include/clang/Basic/CodeGenOptions.h
+++ clang/include/clang/Basic/CodeGenOptions.h
@@ -207,7 +207,10 @@
   std::string RecordCommandLine;
 
   llvm::SmallVector, 0> DebugPrefixMap;
-  std::map CoveragePrefixMap;
+
+  /// Prefix replacement map for source-based code coverage to remap source
+  /// file paths 

[PATCH] D148757: [clang] Apply -fcoverage-prefix-map reverse order

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added inline comments.



Comment at: clang/include/clang/Basic/CodeGenOptions.h:211
+
+  /// Prefix replacement map for coverage.
+  llvm::SmallVector, 0> CoveragePrefixMap;

MaskRay wrote:
> MaskRay wrote:
> > While adding a comment, clarify what coverage it is? There are multiple 
> > coverage instrumentation features in Clang.
> The declaration should not repeat the comment at the use site.
> 
> I am asking for what coverage features this CoveragePrefixMap is used for...
@MaskRay does this sound good to you?
```
/// Prefix replacement map for source-based code coverage to remap source
/// file paths in coverage mapping.
llvm::SmallVector, 0> CoveragePrefixMap;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148757

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

[PATCH] D148757: [clang] Apply -fcoverage-prefix-map reverse order

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem updated this revision to Diff 517375.
gulfem added a comment.

Add text to document that coverage prefix map should be traversed in reverse 
order.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148757

Files:
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Profile/coverage-prefix-map.c


Index: clang/test/Profile/coverage-prefix-map.c
===
--- clang/test/Profile/coverage-prefix-map.c
+++ clang/test/Profile/coverage-prefix-map.c
@@ -19,3 +19,13 @@
 
 // RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -emit-llvm 
-mllvm -enable-name-compression=false -main-file-name coverage-prefix-map.c 
%t/root/nested/coverage-prefix-map.c -fcoverage-compilation-dir=/custom 
-fcoverage-prefix-map=/custom=/nonsense -o - | FileCheck 
--check-prefix=COVERAGE-COMPILATION-DIR %s
 // COVERAGE-COMPILATION-DIR: @__llvm_coverage_mapping = {{.*"\\02.*}}nonsense
+
+// Test that last -fcoverage-prefix-map option 
(-fcoverage-prefix-map==newpath) is applied.
+// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -emit-llvm 
-mllvm -enable-name-compression=false -main-file-name coverage-prefix-map.c 
%t/root/nested/coverage-prefix-map.c -fcoverage-prefix-map=%/t/root=. 
-fcoverage-prefix-map==newpath -o - | FileCheck 
--check-prefix=COVERAGE-PREFIX-MAP-ORDER %s
+// COVERAGE-PREFIX-MAP-ORDER: @__llvm_coverage_mapping = 
{{.*"\\02.*newpath.*root.*nested.*coverage-prefix-map\.c}}
+
+// Test that last -fcoverage-prefix-map option 
(-fcoverage-prefix-map=%/t/root=.) is applied.
+// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -emit-llvm 
-mllvm -enable-name-compression=false -main-file-name coverage-prefix-map.c 
%t/root/nested/coverage-prefix-map.c -fcoverage-prefix-map==newpath 
-fcoverage-prefix-map=%/t/root=. -o - | FileCheck 
--check-prefix=COVERAGE-PREFIX-MAP-REORDER %s
+// COVERAGE-PREFIX-MAP-REORDER: @__llvm_coverage_mapping =
+// COVERAGE-PREFIX-MAP-REORDER-NOT: newpath
+// COVERAGE-PREFIX-MAP-REORDER-SAME: nested{{.*coverage-prefix-map\.c}}
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -1701,8 +1701,7 @@
 
   for (const auto  : Args.getAllArgValues(OPT_fcoverage_prefix_map_EQ)) {
 auto Split = StringRef(Arg).split('=');
-Opts.CoveragePrefixMap.insert(
-{std::string(Split.first), std::string(Split.second)});
+Opts.CoveragePrefixMap.emplace_back(Split.first, Split.second);
   }
 
   const llvm::Triple::ArchType DebugEntryValueArchs[] = {
Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -1648,8 +1648,13 @@
 std::string CoverageMappingModuleGen::normalizeFilename(StringRef Filename) {
   llvm::SmallString<256> Path(Filename);
   llvm::sys::path::remove_dots(Path, /*remove_dot_dot=*/true);
-  for (const auto  : CGM.getCodeGenOpts().CoveragePrefixMap) {
-if (llvm::sys::path::replace_path_prefix(Path, Entry.first, Entry.second))
+
+  /// Traverse coverage prefix map in reverse order because prefix replacements
+  /// are applied in reverse order starting from the last one when multiple
+  /// prefix replacement options are provided.
+  for (const auto &[From, To] :
+   llvm::reverse(CGM.getCodeGenOpts().CoveragePrefixMap)) {
+if (llvm::sys::path::replace_path_prefix(Path, From, To))
   break;
   }
   return Path.str().str();
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -3223,7 +3223,8 @@
 def fcoverage_prefix_map_EQ
   : Joined<["-"], "fcoverage-prefix-map=">, Group,
 Flags<[CC1Option]>,
-HelpText<"remap file source paths in coverage mapping">;
+MetaVarName<"=">,
+HelpText<"remap file source paths  to  in coverage mapping. If 
there are multiple options, prefix replacement is applied in reverse order 
starting from the last one">;
 def ffile_prefix_map_EQ
   : Joined<["-"], "ffile-prefix-map=">, Group,
 HelpText<"remap file source paths in debug info, predefined preprocessor "
Index: clang/include/clang/Basic/CodeGenOptions.h
===
--- clang/include/clang/Basic/CodeGenOptions.h
+++ clang/include/clang/Basic/CodeGenOptions.h
@@ -207,7 +207,11 @@
   std::string RecordCommandLine;
 
   llvm::SmallVector, 0> DebugPrefixMap;
-  std::map CoveragePrefixMap;
+
+  /// Prefix replacement map for source-based code coverage.
+  /// 

[PATCH] D148757: [clang] Apply -fcoverage-prefix-map reverse order

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem updated this revision to Diff 517012.
gulfem marked an inline comment as done.
gulfem added a comment.

Added more info into HelpText and reversed prefix map in the first place to 
incorporate coworker feedback.
GCC uses a linked list to store the prefix mappings in reverse order:
https://github.com/gcc-mirror/gcc/blob/master/gcc/file-prefix-map.cc#L27


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148757

Files:
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Profile/coverage-prefix-map.c


Index: clang/test/Profile/coverage-prefix-map.c
===
--- clang/test/Profile/coverage-prefix-map.c
+++ clang/test/Profile/coverage-prefix-map.c
@@ -19,3 +19,13 @@
 
 // RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -emit-llvm 
-mllvm -enable-name-compression=false -main-file-name coverage-prefix-map.c 
%t/root/nested/coverage-prefix-map.c -fcoverage-compilation-dir=/custom 
-fcoverage-prefix-map=/custom=/nonsense -o - | FileCheck 
--check-prefix=COVERAGE-COMPILATION-DIR %s
 // COVERAGE-COMPILATION-DIR: @__llvm_coverage_mapping = {{.*"\\02.*}}nonsense
+
+// Test that last -fcoverage-prefix-map option 
(-fcoverage-prefix-map==newpath) is applied.
+// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -emit-llvm 
-mllvm -enable-name-compression=false -main-file-name coverage-prefix-map.c 
%t/root/nested/coverage-prefix-map.c -fcoverage-prefix-map=%/t/root=. 
-fcoverage-prefix-map==newpath -o - | FileCheck 
--check-prefix=COVERAGE-PREFIX-MAP-ORDER %s
+// COVERAGE-PREFIX-MAP-ORDER: @__llvm_coverage_mapping = 
{{.*"\\02.*newpath.*root.*nested.*coverage-prefix-map\.c}}
+
+// Test that last -fcoverage-prefix-map option 
(-fcoverage-prefix-map=%/t/root=.) is applied.
+// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -emit-llvm 
-mllvm -enable-name-compression=false -main-file-name coverage-prefix-map.c 
%t/root/nested/coverage-prefix-map.c -fcoverage-prefix-map==newpath 
-fcoverage-prefix-map=%/t/root=. -o - | FileCheck 
--check-prefix=COVERAGE-PREFIX-MAP-REORDER %s
+// COVERAGE-PREFIX-MAP-REORDER: @__llvm_coverage_mapping =
+// COVERAGE-PREFIX-MAP-REORDER-NOT: newpath
+// COVERAGE-PREFIX-MAP-REORDER-SAME: nested{{.*coverage-prefix-map\.c}}
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -1701,9 +1701,12 @@
 
   for (const auto  : Args.getAllArgValues(OPT_fcoverage_prefix_map_EQ)) {
 auto Split = StringRef(Arg).split('=');
-Opts.CoveragePrefixMap.insert(
-{std::string(Split.first), std::string(Split.second)});
+Opts.CoveragePrefixMap.emplace_back(Split.first, Split.second);
   }
+  // Reverse prefix replacement map because if there are multiple options,
+  // the last such option is the one that is effective.
+  if (!Opts.CoveragePrefixMap.empty())
+std::reverse(Opts.CoveragePrefixMap.begin(), Opts.CoveragePrefixMap.end());
 
   const llvm::Triple::ArchType DebugEntryValueArchs[] = {
   llvm::Triple::x86, llvm::Triple::x86_64, llvm::Triple::aarch64,
Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -1648,8 +1648,8 @@
 std::string CoverageMappingModuleGen::normalizeFilename(StringRef Filename) {
   llvm::SmallString<256> Path(Filename);
   llvm::sys::path::remove_dots(Path, /*remove_dot_dot=*/true);
-  for (const auto  : CGM.getCodeGenOpts().CoveragePrefixMap) {
-if (llvm::sys::path::replace_path_prefix(Path, Entry.first, Entry.second))
+  for (const auto &[From, To] : CGM.getCodeGenOpts().CoveragePrefixMap) {
+if (llvm::sys::path::replace_path_prefix(Path, From, To))
   break;
   }
   return Path.str().str();
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -3223,7 +3223,8 @@
 def fcoverage_prefix_map_EQ
   : Joined<["-"], "fcoverage-prefix-map=">, Group,
 Flags<[CC1Option]>,
-HelpText<"remap file source paths in coverage mapping">;
+MetaVarName<"=">,
+HelpText<"remap file source paths  to  in coverage mapping. If 
there are multiple options, the last such option is the one that is effective">;
 def ffile_prefix_map_EQ
   : Joined<["-"], "ffile-prefix-map=">, Group,
 HelpText<"remap file source paths in debug info, predefined preprocessor "
Index: clang/include/clang/Basic/CodeGenOptions.h
===
--- 

[PATCH] D148757: [clang] Apply -fcoverage-prefix-map reverse order

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem marked 2 inline comments as done.
gulfem added inline comments.



Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:1638
 : CGM(CGM), SourceInfo(SourceInfo) {
-  CoveragePrefixMap = CGM.getCodeGenOpts().CoveragePrefixMap;
+  for (const auto &[From, To] : CGM.getCodeGenOpts().CoveragePrefixMap)
+CoveragePrefixMap.emplace_back(From, To);

MaskRay wrote:
> `CoveragePrefixMap` can be removed. I just removed `DebugPrefixMap` as well:)
I'm marking this `Done` as you removed it in 
https://github.com/llvm/llvm-project/commit/d3e121780ac2724969a030415f3092170f3c7589.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148757

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

[PATCH] D148757: [clang] Apply -fcoverage-prefix-map reverse order

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem updated this revision to Diff 516993.
gulfem added a comment.

Added a comment about reverse traversal.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148757

Files:
  clang/include/clang/Basic/CodeGenOptions.h
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Profile/coverage-prefix-map.c


Index: clang/test/Profile/coverage-prefix-map.c
===
--- clang/test/Profile/coverage-prefix-map.c
+++ clang/test/Profile/coverage-prefix-map.c
@@ -19,3 +19,13 @@
 
 // RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -emit-llvm 
-mllvm -enable-name-compression=false -main-file-name coverage-prefix-map.c 
%t/root/nested/coverage-prefix-map.c -fcoverage-compilation-dir=/custom 
-fcoverage-prefix-map=/custom=/nonsense -o - | FileCheck 
--check-prefix=COVERAGE-COMPILATION-DIR %s
 // COVERAGE-COMPILATION-DIR: @__llvm_coverage_mapping = {{.*"\\02.*}}nonsense
+
+// Test that last -fcoverage-prefix-map option 
(-fcoverage-prefix-map==newpath) is applied.
+// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -emit-llvm 
-mllvm -enable-name-compression=false -main-file-name coverage-prefix-map.c 
%t/root/nested/coverage-prefix-map.c -fcoverage-prefix-map=%/t/root=. 
-fcoverage-prefix-map==newpath -o - | FileCheck 
--check-prefix=COVERAGE-PREFIX-MAP-ORDER %s
+// COVERAGE-PREFIX-MAP-ORDER: @__llvm_coverage_mapping = 
{{.*"\\02.*newpath.*root.*nested.*coverage-prefix-map\.c}}
+
+// Test that last -fcoverage-prefix-map option 
(-fcoverage-prefix-map=%/t/root=.) is applied.
+// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -emit-llvm 
-mllvm -enable-name-compression=false -main-file-name coverage-prefix-map.c 
%t/root/nested/coverage-prefix-map.c -fcoverage-prefix-map==newpath 
-fcoverage-prefix-map=%/t/root=. -o - | FileCheck 
--check-prefix=COVERAGE-PREFIX-MAP-REORDER %s
+// COVERAGE-PREFIX-MAP-REORDER: @__llvm_coverage_mapping =
+// COVERAGE-PREFIX-MAP-REORDER-NOT: newpath
+// COVERAGE-PREFIX-MAP-REORDER-SAME: nested{{.*coverage-prefix-map\.c}}
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -1701,8 +1701,7 @@
 
   for (const auto  : Args.getAllArgValues(OPT_fcoverage_prefix_map_EQ)) {
 auto Split = StringRef(Arg).split('=');
-Opts.CoveragePrefixMap.insert(
-{std::string(Split.first), std::string(Split.second)});
+Opts.CoveragePrefixMap.emplace_back(Split.first, Split.second);
   }
 
   const llvm::Triple::ArchType DebugEntryValueArchs[] = {
Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -1648,8 +1648,12 @@
 std::string CoverageMappingModuleGen::normalizeFilename(StringRef Filename) {
   llvm::SmallString<256> Path(Filename);
   llvm::sys::path::remove_dots(Path, /*remove_dot_dot=*/true);
-  for (const auto  : CGM.getCodeGenOpts().CoveragePrefixMap) {
-if (llvm::sys::path::replace_path_prefix(Path, Entry.first, Entry.second))
+
+  // Traverse coverage prefix replacement mapping options that are provided in
+  // reverse order to apply them.
+  for (const auto &[From, To] :
+   llvm::reverse(CGM.getCodeGenOpts().CoveragePrefixMap)) {
+if (llvm::sys::path::replace_path_prefix(Path, From, To))
   break;
   }
   return Path.str().str();
Index: clang/include/clang/Basic/CodeGenOptions.h
===
--- clang/include/clang/Basic/CodeGenOptions.h
+++ clang/include/clang/Basic/CodeGenOptions.h
@@ -207,7 +207,9 @@
   std::string RecordCommandLine;
 
   llvm::SmallVector, 0> DebugPrefixMap;
-  std::map CoveragePrefixMap;
+
+  /// Prefix replacement map for source-based code coverage.
+  llvm::SmallVector, 0> CoveragePrefixMap;
 
   /// The ABI to use for passing floating point arguments.
   std::string FloatABI;


Index: clang/test/Profile/coverage-prefix-map.c
===
--- clang/test/Profile/coverage-prefix-map.c
+++ clang/test/Profile/coverage-prefix-map.c
@@ -19,3 +19,13 @@
 
 // RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -emit-llvm -mllvm -enable-name-compression=false -main-file-name coverage-prefix-map.c %t/root/nested/coverage-prefix-map.c -fcoverage-compilation-dir=/custom -fcoverage-prefix-map=/custom=/nonsense -o - | FileCheck --check-prefix=COVERAGE-COMPILATION-DIR %s
 // COVERAGE-COMPILATION-DIR: @__llvm_coverage_mapping = {{.*"\\02.*}}nonsense
+
+// Test that last -fcoverage-prefix-map option (-fcoverage-prefix-map==newpath) is applied.
+// RUN: %clang_cc1 -fprofile-instrument=clang 

[PATCH] D148757: [clang] Apply -fcoverage-prefix-map reverse order

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem updated this revision to Diff 516989.
gulfem added a comment.

Addressed feedback and rebased.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148757

Files:
  clang/include/clang/Basic/CodeGenOptions.h
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Profile/coverage-prefix-map.c


Index: clang/test/Profile/coverage-prefix-map.c
===
--- clang/test/Profile/coverage-prefix-map.c
+++ clang/test/Profile/coverage-prefix-map.c
@@ -19,3 +19,13 @@
 
 // RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -emit-llvm 
-mllvm -enable-name-compression=false -main-file-name coverage-prefix-map.c 
%t/root/nested/coverage-prefix-map.c -fcoverage-compilation-dir=/custom 
-fcoverage-prefix-map=/custom=/nonsense -o - | FileCheck 
--check-prefix=COVERAGE-COMPILATION-DIR %s
 // COVERAGE-COMPILATION-DIR: @__llvm_coverage_mapping = {{.*"\\02.*}}nonsense
+
+// Test that last -fcoverage-prefix-map option 
(-fcoverage-prefix-map==newpath) is applied.
+// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -emit-llvm 
-mllvm -enable-name-compression=false -main-file-name coverage-prefix-map.c 
%t/root/nested/coverage-prefix-map.c -fcoverage-prefix-map=%/t/root=. 
-fcoverage-prefix-map==newpath -o - | FileCheck 
--check-prefix=COVERAGE-PREFIX-MAP-ORDER %s
+// COVERAGE-PREFIX-MAP-ORDER: @__llvm_coverage_mapping = 
{{.*"\\02.*newpath.*root.*nested.*coverage-prefix-map\.c}}
+
+// Test that last -fcoverage-prefix-map option 
(-fcoverage-prefix-map=%/t/root=.) is applied.
+// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -emit-llvm 
-mllvm -enable-name-compression=false -main-file-name coverage-prefix-map.c 
%t/root/nested/coverage-prefix-map.c -fcoverage-prefix-map==newpath 
-fcoverage-prefix-map=%/t/root=. -o - | FileCheck 
--check-prefix=COVERAGE-PREFIX-MAP-REORDER %s
+// COVERAGE-PREFIX-MAP-REORDER: @__llvm_coverage_mapping =
+// COVERAGE-PREFIX-MAP-REORDER-NOT: newpath
+// COVERAGE-PREFIX-MAP-REORDER-SAME: nested{{.*coverage-prefix-map\.c}}
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -1701,8 +1701,7 @@
 
   for (const auto  : Args.getAllArgValues(OPT_fcoverage_prefix_map_EQ)) {
 auto Split = StringRef(Arg).split('=');
-Opts.CoveragePrefixMap.insert(
-{std::string(Split.first), std::string(Split.second)});
+Opts.CoveragePrefixMap.emplace_back(Split.first, Split.second);
   }
 
   const llvm::Triple::ArchType DebugEntryValueArchs[] = {
Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -1648,8 +1648,9 @@
 std::string CoverageMappingModuleGen::normalizeFilename(StringRef Filename) {
   llvm::SmallString<256> Path(Filename);
   llvm::sys::path::remove_dots(Path, /*remove_dot_dot=*/true);
-  for (const auto  : CGM.getCodeGenOpts().CoveragePrefixMap) {
-if (llvm::sys::path::replace_path_prefix(Path, Entry.first, Entry.second))
+  for (const auto &[From, To] :
+   llvm::reverse(CGM.getCodeGenOpts().CoveragePrefixMap)) {
+if (llvm::sys::path::replace_path_prefix(Path, From, To))
   break;
   }
   return Path.str().str();
Index: clang/include/clang/Basic/CodeGenOptions.h
===
--- clang/include/clang/Basic/CodeGenOptions.h
+++ clang/include/clang/Basic/CodeGenOptions.h
@@ -207,7 +207,9 @@
   std::string RecordCommandLine;
 
   llvm::SmallVector, 0> DebugPrefixMap;
-  std::map CoveragePrefixMap;
+
+  /// Prefix replacement map for source-based code coverage.
+  llvm::SmallVector, 0> CoveragePrefixMap;
 
   /// The ABI to use for passing floating point arguments.
   std::string FloatABI;


Index: clang/test/Profile/coverage-prefix-map.c
===
--- clang/test/Profile/coverage-prefix-map.c
+++ clang/test/Profile/coverage-prefix-map.c
@@ -19,3 +19,13 @@
 
 // RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -emit-llvm -mllvm -enable-name-compression=false -main-file-name coverage-prefix-map.c %t/root/nested/coverage-prefix-map.c -fcoverage-compilation-dir=/custom -fcoverage-prefix-map=/custom=/nonsense -o - | FileCheck --check-prefix=COVERAGE-COMPILATION-DIR %s
 // COVERAGE-COMPILATION-DIR: @__llvm_coverage_mapping = {{.*"\\02.*}}nonsense
+
+// Test that last -fcoverage-prefix-map option (-fcoverage-prefix-map==newpath) is applied.
+// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -emit-llvm -mllvm -enable-name-compression=false -main-file-name coverage-prefix-map.c 

[PATCH] D148757: [clang] Apply last -fcoverage-prefix-map option

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem updated this revision to Diff 516853.
gulfem added a comment.

Updated the text.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148757

Files:
  clang/include/clang/Basic/CodeGenOptions.h
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/CodeGen/CoverageMappingGen.h
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Profile/coverage-prefix-map.c


Index: clang/test/Profile/coverage-prefix-map.c
===
--- clang/test/Profile/coverage-prefix-map.c
+++ clang/test/Profile/coverage-prefix-map.c
@@ -19,3 +19,13 @@
 
 // RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -emit-llvm 
-mllvm -enable-name-compression=false -main-file-name coverage-prefix-map.c 
%t/root/nested/coverage-prefix-map.c -fcoverage-compilation-dir=/custom 
-fcoverage-prefix-map=/custom=/nonsense -o - | FileCheck 
--check-prefix=COVERAGE-COMPILATION-DIR %s
 // COVERAGE-COMPILATION-DIR: @__llvm_coverage_mapping = {{.*"\\02.*}}nonsense
+
+// Test that last -fcoverage-prefix-map option 
(-fcoverage-prefix-map==newpath) is applied.
+// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -emit-llvm 
-mllvm -enable-name-compression=false -main-file-name coverage-prefix-map.c 
%t/root/nested/coverage-prefix-map.c -fcoverage-prefix-map=%/t/root=. 
-fcoverage-prefix-map==newpath -o - | FileCheck 
--check-prefix=COVERAGE-PREFIX-MAP-ORDER %s
+// COVERAGE-PREFIX-MAP-ORDER: @__llvm_coverage_mapping = 
{{.*"\\02.*newpath.*root.*nested.*coverage-prefix-map\.c}}
+
+// Test that last -fcoverage-prefix-map option 
(-fcoverage-prefix-map=%/t/root=.) is applied.
+// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -emit-llvm 
-mllvm -enable-name-compression=false -main-file-name coverage-prefix-map.c 
%t/root/nested/coverage-prefix-map.c -fcoverage-prefix-map==newpath 
-fcoverage-prefix-map=%/t/root=. -o - | FileCheck 
--check-prefix=COVERAGE-PREFIX-MAP-REORDER %s
+// COVERAGE-PREFIX-MAP-REORDER: @__llvm_coverage_mapping =
+// COVERAGE-PREFIX-MAP-REORDER-NOT: newpath
+// COVERAGE-PREFIX-MAP-REORDER-SAME: nested{{.*coverage-prefix-map\.c}}
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -1702,8 +1702,7 @@
 
   for (const auto  : Args.getAllArgValues(OPT_fcoverage_prefix_map_EQ)) {
 auto Split = StringRef(Arg).split('=');
-Opts.CoveragePrefixMap.insert(
-{std::string(Split.first), std::string(Split.second)});
+Opts.CoveragePrefixMap.emplace_back(Split.first, Split.second);
   }
 
   const llvm::Triple::ArchType DebugEntryValueArchs[] = {
Index: clang/lib/CodeGen/CoverageMappingGen.h
===
--- clang/lib/CodeGen/CoverageMappingGen.h
+++ clang/lib/CodeGen/CoverageMappingGen.h
@@ -107,7 +107,10 @@
   llvm::SmallDenseMap FileEntries;
   std::vector FunctionNames;
   std::vector FunctionRecords;
-  std::map CoveragePrefixMap;
+
+  /// Prefix replacement map for coverage.
+  llvm::SmallVector, 0>
+  CoveragePrefixMap;
 
   std::string getCurrentDirname();
   std::string normalizeFilename(StringRef Filename);
Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -1635,7 +1635,8 @@
 CoverageMappingModuleGen::CoverageMappingModuleGen(
 CodeGenModule , CoverageSourceInfo )
 : CGM(CGM), SourceInfo(SourceInfo) {
-  CoveragePrefixMap = CGM.getCodeGenOpts().CoveragePrefixMap;
+  for (const auto &[From, To] : CGM.getCodeGenOpts().CoveragePrefixMap)
+CoveragePrefixMap.emplace_back(From, To);
 }
 
 std::string CoverageMappingModuleGen::getCurrentDirname() {
@@ -1650,10 +1651,13 @@
 std::string CoverageMappingModuleGen::normalizeFilename(StringRef Filename) {
   llvm::SmallString<256> Path(Filename);
   llvm::sys::path::remove_dots(Path, /*remove_dot_dot=*/true);
-  for (const auto  : CoveragePrefixMap) {
-if (llvm::sys::path::replace_path_prefix(Path, Entry.first, Entry.second))
+
+  // Traverse coverage prefix mapping options that are provided in reverse
+  // order.
+  for (const auto &[From, To] : llvm::reverse(CoveragePrefixMap))
+if (llvm::sys::path::replace_path_prefix(Path, From, To))
   break;
-  }
+
   return Path.str().str();
 }
 
Index: clang/include/clang/Basic/CodeGenOptions.h
===
--- clang/include/clang/Basic/CodeGenOptions.h
+++ clang/include/clang/Basic/CodeGenOptions.h
@@ -207,7 +207,9 @@
   std::string RecordCommandLine;
 
   std::map DebugPrefixMap;
-  std::map CoveragePrefixMap;
+
+  /// Prefix replacement map for coverage.
+  llvm::SmallVector, 0> CoveragePrefixMap;
 
   /// The 

[PATCH] D148757: [clang] Follow user-provided order for prefix map

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

In D148757#4289152 , @MaskRay wrote:

> In D148757#4289076 , @MaskRay wrote:
>
>> I'm creating a patch to change DebugPrefixMap to respect the command line 
>> option order...
>
> Created D148975  to follow GCC's behavior 
> (the last applicable option wins).
>
> In this patch, you may drop the `DebugPrefixMap` 
> (clang/include/clang/Basic/CodeGenOptions.h) change (a no-op without other 
> changes to `DebugPrefixMap` ).

Thanks for you guidance on this @MaskRay, and I removed the changes to 
debuginfo in the new revision.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148757

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

[PATCH] D148757: [clang] Follow user-provided order for prefix map

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem updated this revision to Diff 516616.
gulfem added a comment.

Remove the changes from debuginfo, which is handled in D14897 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148757

Files:
  clang/include/clang/Basic/CodeGenOptions.h
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/CodeGen/CoverageMappingGen.h
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Profile/coverage-prefix-map.c


Index: clang/test/Profile/coverage-prefix-map.c
===
--- clang/test/Profile/coverage-prefix-map.c
+++ clang/test/Profile/coverage-prefix-map.c
@@ -19,3 +19,13 @@
 
 // RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -emit-llvm 
-mllvm -enable-name-compression=false -main-file-name coverage-prefix-map.c 
%t/root/nested/coverage-prefix-map.c -fcoverage-compilation-dir=/custom 
-fcoverage-prefix-map=/custom=/nonsense -o - | FileCheck 
--check-prefix=COVERAGE-COMPILATION-DIR %s
 // COVERAGE-COMPILATION-DIR: @__llvm_coverage_mapping = {{.*"\\02.*}}nonsense
+
+// Test that last -fcoverage-prefix-map option 
(-fcoverage-prefix-map==newpath) is applied.
+// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -emit-llvm 
-mllvm -enable-name-compression=false -main-file-name coverage-prefix-map.c 
%t/root/nested/coverage-prefix-map.c -fcoverage-prefix-map=%/t/root=. 
-fcoverage-prefix-map==newpath -o - | FileCheck 
--check-prefix=COVERAGE-PREFIX-MAP-ORDER %s
+// COVERAGE-PREFIX-MAP-ORDER: @__llvm_coverage_mapping = 
{{.*"\\02.*newpath.*root.*nested.*coverage-prefix-map\.c}}
+
+// Test that last -fcoverage-prefix-map option 
(-fcoverage-prefix-map=%/t/root=.) is applied.
+// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -emit-llvm 
-mllvm -enable-name-compression=false -main-file-name coverage-prefix-map.c 
%t/root/nested/coverage-prefix-map.c -fcoverage-prefix-map==newpath 
-fcoverage-prefix-map=%/t/root=. -o - | FileCheck 
--check-prefix=COVERAGE-PREFIX-MAP-REORDER %s
+// COVERAGE-PREFIX-MAP-REORDER: @__llvm_coverage_mapping =
+// COVERAGE-PREFIX-MAP-REORDER-NOT: newpath
+// COVERAGE-PREFIX-MAP-REORDER-SAME: nested{{.*coverage-prefix-map\.c}}
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -1702,8 +1702,7 @@
 
   for (const auto  : Args.getAllArgValues(OPT_fcoverage_prefix_map_EQ)) {
 auto Split = StringRef(Arg).split('=');
-Opts.CoveragePrefixMap.insert(
-{std::string(Split.first), std::string(Split.second)});
+Opts.CoveragePrefixMap.emplace_back(Split.first, Split.second);
   }
 
   const llvm::Triple::ArchType DebugEntryValueArchs[] = {
Index: clang/lib/CodeGen/CoverageMappingGen.h
===
--- clang/lib/CodeGen/CoverageMappingGen.h
+++ clang/lib/CodeGen/CoverageMappingGen.h
@@ -107,7 +107,10 @@
   llvm::SmallDenseMap FileEntries;
   std::vector FunctionNames;
   std::vector FunctionRecords;
-  std::map CoveragePrefixMap;
+
+  /// Prefix replacement map for coverage.
+  llvm::SmallVector, 0>
+  CoveragePrefixMap;
 
   std::string getCurrentDirname();
   std::string normalizeFilename(StringRef Filename);
Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -1635,7 +1635,8 @@
 CoverageMappingModuleGen::CoverageMappingModuleGen(
 CodeGenModule , CoverageSourceInfo )
 : CGM(CGM), SourceInfo(SourceInfo) {
-  CoveragePrefixMap = CGM.getCodeGenOpts().CoveragePrefixMap;
+  for (const auto &[From, To] : CGM.getCodeGenOpts().CoveragePrefixMap)
+CoveragePrefixMap.emplace_back(From, To);
 }
 
 std::string CoverageMappingModuleGen::getCurrentDirname() {
@@ -1650,10 +1651,14 @@
 std::string CoverageMappingModuleGen::normalizeFilename(StringRef Filename) {
   llvm::SmallString<256> Path(Filename);
   llvm::sys::path::remove_dots(Path, /*remove_dot_dot=*/true);
-  for (const auto  : CoveragePrefixMap) {
-if (llvm::sys::path::replace_path_prefix(Path, Entry.first, Entry.second))
+
+  // Traverse CoveragePrefixMap in reverse order to apply the last provided 
path
+  // prefix replacement when multiple -fcoverage-prefix-map options are
+  // provided.
+  for (const auto &[From, To] : llvm::reverse(CoveragePrefixMap))
+if (llvm::sys::path::replace_path_prefix(Path, From, To))
   break;
-  }
+
   return Path.str().str();
 }
 
Index: clang/include/clang/Basic/CodeGenOptions.h
===
--- clang/include/clang/Basic/CodeGenOptions.h
+++ clang/include/clang/Basic/CodeGenOptions.h
@@ -207,7 +207,9 @@
   std::string RecordCommandLine;
 

[PATCH] D148757: [clang] Follow user-provided order for prefix map

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

I compared Clang and GCC behavior.
**1) Following user-specified prefix mapping order**
GCC uses a linked list to store the prefix mappings, so it applies the prefix 
mappings based on the user-provided order.
https://github.com/gcc-mirror/gcc/blob/master/gcc/file-prefix-map.cc#L27
Whereas, Clang currently uses an `std::map`, which does **NOT** follow 
user-provided order.  So, Clang and GCC behavior do not match on that.

**2) Applying multiple prefix mappings**
Both Clang and GCC apply the first matching prefix mapping, and does not apply 
the following prefix mappings. So, they are consistent in that.

https://github.com/gcc-mirror/gcc/blob/master/gcc/file-prefix-map.cc#L88
https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CoverageMappingGen.cpp#L1653


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148757

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

[PATCH] D148757: [clang] Follow user-provided order for prefix map

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem updated this revision to Diff 515560.
gulfem added a comment.

Modified the test case


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148757

Files:
  clang/include/clang/Basic/CodeGenOptions.h
  clang/lib/CodeGen/CoverageMappingGen.h
  clang/test/Profile/coverage-prefix-map.c


Index: clang/test/Profile/coverage-prefix-map.c
===
--- clang/test/Profile/coverage-prefix-map.c
+++ clang/test/Profile/coverage-prefix-map.c
@@ -19,3 +19,13 @@
 
 // RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -emit-llvm 
-mllvm -enable-name-compression=false -main-file-name coverage-prefix-map.c 
%t/root/nested/coverage-prefix-map.c -fcoverage-compilation-dir=/custom 
-fcoverage-prefix-map=/custom=/nonsense -o - | FileCheck 
--check-prefix=COVERAGE-COMPILATION-DIR %s
 // COVERAGE-COMPILATION-DIR: @__llvm_coverage_mapping = {{.*"\\02.*}}nonsense
+
+// Test that user provided prefix replacement order is followed 
(-fcoverage-prefix-map==newpath is applied because it is the first one in the 
user-provided prefix-map order).
+// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -emit-llvm 
-mllvm -enable-name-compression=false -main-file-name coverage-prefix-map.c 
%t/root/nested/coverage-prefix-map.c -fcoverage-prefix-map==newpath 
-fcoverage-prefix-map=%/t/root=. -o - | FileCheck 
--check-prefix=COVERAGE-PREFIX-MAP-ORDER %s
+// COVERAGE-PREFIX-MAP-ORDER: @__llvm_coverage_mapping = 
{{.*"\\02.*newpath.*root.*nested.*coverage-prefix-map\.c}}
+
+// Test that -fcoverage-prefix-map=%/t/root=. is applied but 
-fcoverage-prefix-map==newpath is NOT because of the order.
+// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -emit-llvm 
-mllvm -enable-name-compression=false -main-file-name coverage-prefix-map.c 
%t/root/nested/coverage-prefix-map.c -fcoverage-prefix-map=%/t/root=. 
-fcoverage-prefix-map==newpath -o - | FileCheck 
--check-prefix=COVERAGE-PREFIX-MAP-REORDER %s
+// COVERAGE-PREFIX-MAP-REORDER: @__llvm_coverage_mapping =
+// COVERAGE-PREFIX-MAP-REORDER-NOT: newpath
+// COVERAGE-PREFIX-MAP-REORDER-SAME: nested{{.*coverage-prefix-map\.c}}
Index: clang/lib/CodeGen/CoverageMappingGen.h
===
--- clang/lib/CodeGen/CoverageMappingGen.h
+++ clang/lib/CodeGen/CoverageMappingGen.h
@@ -18,6 +18,8 @@
 #include "clang/Lex/PPCallbacks.h"
 #include "clang/Lex/Preprocessor.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/MapVector.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/IR/GlobalValue.h"
 #include "llvm/Support/raw_ostream.h"
 
@@ -107,7 +109,8 @@
   llvm::SmallDenseMap FileEntries;
   std::vector FunctionNames;
   std::vector FunctionRecords;
-  std::map CoveragePrefixMap;
+  llvm::MapVector>
+  CoveragePrefixMap;
 
   std::string getCurrentDirname();
   std::string normalizeFilename(StringRef Filename);
Index: clang/include/clang/Basic/CodeGenOptions.h
===
--- clang/include/clang/Basic/CodeGenOptions.h
+++ clang/include/clang/Basic/CodeGenOptions.h
@@ -17,6 +17,8 @@
 #include "clang/Basic/XRayInstr.h"
 #include "llvm/ADT/FloatingPointMode.h"
 #include "llvm/Frontend/Debug/Options.h"
+#include "llvm/ADT/MapVector.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/Support/CodeGen.h"
 #include "llvm/Support/Regex.h"
 #include "llvm/Target/TargetOptions.h"
@@ -206,8 +208,13 @@
   /// if non-empty.
   std::string RecordCommandLine;
 
-  std::map DebugPrefixMap;
-  std::map CoveragePrefixMap;
+  /// Prefix replacement map for coverage.
+  llvm::MapVector>
+  CoveragePrefixMap;
+
+  /// Prefix replacement map for debug info.
+  llvm::MapVector>
+  DebugPrefixMap;
 
   /// The ABI to use for passing floating point arguments.
   std::string FloatABI;


Index: clang/test/Profile/coverage-prefix-map.c
===
--- clang/test/Profile/coverage-prefix-map.c
+++ clang/test/Profile/coverage-prefix-map.c
@@ -19,3 +19,13 @@
 
 // RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -emit-llvm -mllvm -enable-name-compression=false -main-file-name coverage-prefix-map.c %t/root/nested/coverage-prefix-map.c -fcoverage-compilation-dir=/custom -fcoverage-prefix-map=/custom=/nonsense -o - | FileCheck --check-prefix=COVERAGE-COMPILATION-DIR %s
 // COVERAGE-COMPILATION-DIR: @__llvm_coverage_mapping = {{.*"\\02.*}}nonsense
+
+// Test that user provided prefix replacement order is followed (-fcoverage-prefix-map==newpath is applied because it is the first one in the user-provided prefix-map order).
+// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -emit-llvm -mllvm -enable-name-compression=false -main-file-name coverage-prefix-map.c %t/root/nested/coverage-prefix-map.c -fcoverage-prefix-map==newpath -fcoverage-prefix-map=%/t/root=. -o - 

[PATCH] D148757: [clang] Follow user-provided order for prefix map

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

In D148757#4282002 , @MaskRay wrote:

> Can you add example options in the description and say how this patch is 
> going to change it?
>
> I think `std::map` is intended so that for `-f*-prefix-map` values, if both 
> `a/b=...` and `a/b/c=...` match an input path, the longest wins, not the 
> latest wins.

We discovered the issue in the context of supporting source-based code coverage 
in the Pigweed project.
The project provides two `-ffile-prefix-map` paths that can impact coverage: 
https://cs.opensource.google/pigweed/pigweed/+/main:pw_build/BUILD.gn;l=251-256

1. `-ffile-prefix-map=../= ` to remove the relative path from the build 
directory to the root
2. `-ffile-prefix-map==out/` to prepend out/ to any unmatched files

If the user-specified order is applied, 1) should be applied, but in this case 
2) is applied because the std::map iteration order is not the same of the 
insertion order.
Applying 2) appends `out` to beginning of file paths in coverage mapping 
records.

The question that we need to answer is whether following the user-specified 
order for `-f*-prefix-map` is the expected behavior.
If that's the case, `Clang` does not follow that.
https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CoverageMappingGen.cpp#L1653


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148757

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

[PATCH] D148757: [clang] Follow user-provided order for prefix map

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem created this revision.
Herald added a project: All.
gulfem requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This patch uses llvm::MapVector instead of std::map to ensure
that iteration order is the insertion order for file prefix remapping
for coverage and debug info in order to follow the prefix remapping
order that the user specifies.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D148757

Files:
  clang/include/clang/Basic/CodeGenOptions.h
  clang/lib/CodeGen/CoverageMappingGen.h
  clang/test/Profile/coverage-prefix-map.c


Index: clang/test/Profile/coverage-prefix-map.c
===
--- clang/test/Profile/coverage-prefix-map.c
+++ clang/test/Profile/coverage-prefix-map.c
@@ -19,3 +19,7 @@
 
 // RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -emit-llvm 
-mllvm -enable-name-compression=false -main-file-name coverage-prefix-map.c 
%t/root/nested/coverage-prefix-map.c -fcoverage-compilation-dir=/custom 
-fcoverage-prefix-map=/custom=/nonsense -o - | FileCheck 
--check-prefix=COVERAGE-COMPILATION-DIR %s
 // COVERAGE-COMPILATION-DIR: @__llvm_coverage_mapping = {{.*"\\02.*}}nonsense
+
+// Test that user provided prefix replacement order is followed by appending 
to nonse to the beginning of every path.
+// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -emit-llvm 
-mllvm -enable-name-compression=false -main-file-name coverage-prefix-map.c 
%t/root/nested/coverage-prefix-map.c -fcoverage-prefix-map==nonsense 
-fcoverage-prefix-map=%/t/root=. -o - | FileCheck 
--check-prefix=COVERAGE-PREFIX-MAP-ORDER %s
+// COVERAGE-PREFIX-MAP-ORDER: @__llvm_coverage_mapping = 
{{.*"\\02.*nonsense.*nonsense.*root.*nested.*coverage-prefix-map\.c}}
Index: clang/lib/CodeGen/CoverageMappingGen.h
===
--- clang/lib/CodeGen/CoverageMappingGen.h
+++ clang/lib/CodeGen/CoverageMappingGen.h
@@ -18,6 +18,8 @@
 #include "clang/Lex/PPCallbacks.h"
 #include "clang/Lex/Preprocessor.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/MapVector.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/IR/GlobalValue.h"
 #include "llvm/Support/raw_ostream.h"
 
@@ -107,7 +109,8 @@
   llvm::SmallDenseMap FileEntries;
   std::vector FunctionNames;
   std::vector FunctionRecords;
-  std::map CoveragePrefixMap;
+  llvm::MapVector>
+  CoveragePrefixMap;
 
   std::string getCurrentDirname();
   std::string normalizeFilename(StringRef Filename);
Index: clang/include/clang/Basic/CodeGenOptions.h
===
--- clang/include/clang/Basic/CodeGenOptions.h
+++ clang/include/clang/Basic/CodeGenOptions.h
@@ -17,6 +17,8 @@
 #include "clang/Basic/XRayInstr.h"
 #include "llvm/ADT/FloatingPointMode.h"
 #include "llvm/Frontend/Debug/Options.h"
+#include "llvm/ADT/MapVector.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/Support/CodeGen.h"
 #include "llvm/Support/Regex.h"
 #include "llvm/Target/TargetOptions.h"
@@ -206,8 +208,13 @@
   /// if non-empty.
   std::string RecordCommandLine;
 
-  std::map DebugPrefixMap;
-  std::map CoveragePrefixMap;
+  /// Prefix replacement map for coverage.
+  llvm::MapVector>
+  CoveragePrefixMap;
+
+  /// Prefix replacement map for debug info.
+  llvm::MapVector>
+  DebugPrefixMap;
 
   /// The ABI to use for passing floating point arguments.
   std::string FloatABI;


Index: clang/test/Profile/coverage-prefix-map.c
===
--- clang/test/Profile/coverage-prefix-map.c
+++ clang/test/Profile/coverage-prefix-map.c
@@ -19,3 +19,7 @@
 
 // RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -emit-llvm -mllvm -enable-name-compression=false -main-file-name coverage-prefix-map.c %t/root/nested/coverage-prefix-map.c -fcoverage-compilation-dir=/custom -fcoverage-prefix-map=/custom=/nonsense -o - | FileCheck --check-prefix=COVERAGE-COMPILATION-DIR %s
 // COVERAGE-COMPILATION-DIR: @__llvm_coverage_mapping = {{.*"\\02.*}}nonsense
+
+// Test that user provided prefix replacement order is followed by appending to nonse to the beginning of every path.
+// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -emit-llvm -mllvm -enable-name-compression=false -main-file-name coverage-prefix-map.c %t/root/nested/coverage-prefix-map.c -fcoverage-prefix-map==nonsense -fcoverage-prefix-map=%/t/root=. -o - | FileCheck --check-prefix=COVERAGE-PREFIX-MAP-ORDER %s
+// COVERAGE-PREFIX-MAP-ORDER: @__llvm_coverage_mapping = {{.*"\\02.*nonsense.*nonsense.*root.*nested.*coverage-prefix-map\.c}}
Index: clang/lib/CodeGen/CoverageMappingGen.h
===
--- clang/lib/CodeGen/CoverageMappingGen.h
+++ clang/lib/CodeGen/CoverageMappingGen.h
@@ -18,6 +18,8 @@
 #include "clang/Lex/PPCallbacks.h"
 #include "clang/Lex/Preprocessor.h"
 #include 

[PATCH] D145646: [clang][driver] Enable '-flto' on AVR

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

In D145646#4213975 , @MaskRay wrote:

> In D145646#4213547 , @gulfem wrote:
>
>> We started seeing a test failure in `avr-ld.c`.
>>
>>   /b/s/w/ir/x/w/llvm-llvm-project/clang/test/Driver/avr-ld.c:48:11: error: 
>>   // LINKP: {{".*ld.*"}} {{.*}} "--defsym=__DATA_REGION_ORIGIN__=0x800100" 
>> "-plugin" {{.*}} "-plugin-opt=mcpu=atmega328"
>> ^
>>   :1:1: note: scanning from here
>>   Fuchsia clang version 17.0.0 (https://llvm.googlesource.com/llvm-project 
>> 0d37efdbc599e61ce2a0418723a66d6b45aea8d7)
>>   ^
>>   :6:239: note: possible intended match here
>>
>> https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8785950851645447937/overview
>
> I am testing a change and will fix it in one minute.

Thanks, and it fixed the test failure that we have been seeing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145646

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

[PATCH] D145646: [clang][driver] Enable '-flto' on AVR

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

We started seeing a test failure in `avr-ld.c`.

  /b/s/w/ir/x/w/llvm-llvm-project/clang/test/Driver/avr-ld.c:48:11: error: 
  // LINKP: {{".*ld.*"}} {{.*}} "--defsym=__DATA_REGION_ORIGIN__=0x800100" 
"-plugin" {{.*}} "-plugin-opt=mcpu=atmega328"
^
  :1:1: note: scanning from here
  Fuchsia clang version 17.0.0 (https://llvm.googlesource.com/llvm-project 
0d37efdbc599e61ce2a0418723a66d6b45aea8d7)
  ^
  :6:239: note: possible intended match here

https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8785950851645447937/overview


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145646

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

[PATCH] D143304: [Coverage] Map regions from system headers

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG2155195131a5: [Coverage] Map regions from system headers 
(authored by gulfem).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143304

Files:
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/test/CoverageMapping/system_macro.cpp


Index: clang/test/CoverageMapping/system_macro.cpp
===
--- clang/test/CoverageMapping/system_macro.cpp
+++ clang/test/CoverageMapping/system_macro.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -mllvm -emptyline-comment-coverage=false -std=c++11 
-fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping 
-emit-llvm-only -main-file-name system_macro.cpp -o - %s | FileCheck %s
+// RUN: %clang_cc1 -mllvm -emptyline-comment-coverage=false -mllvm 
-system-headers-coverage -std=c++11 -fprofile-instrument=clang 
-fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name 
system_macro.cpp -o - %s | FileCheck %s
 
 #ifdef IS_SYSHEADER
 
@@ -13,8 +13,9 @@
 
 // CHECK-LABEL: doSomething
 void doSomething(int x) { // CHECK: File 0, [[@LINE]]:25 -> {{[0-9:]+}} = #0
-  Func(x);
+  Func(x); // CHECK: Expansion,File 0, [[@LINE]]:3 -> [[@LINE]]:7
   return;
+  // CHECK: Expansion,File 0, [[@LINE+1]]:3 -> [[@LINE+1]]:11
   SomeType *f; // CHECK: File 0, [[@LINE]]:11 -> {{[0-9:]+}} = 0
 }
 
Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -37,6 +37,11 @@
"disable it on test)"),
 llvm::cl::init(true), llvm::cl::Hidden);
 
+static llvm::cl::opt SystemHeadersCoverage(
+"system-headers-coverage",
+llvm::cl::desc("Enable collecting coverage from system headers"),
+llvm::cl::init(false), llvm::cl::Hidden);
+
 using namespace clang;
 using namespace CodeGen;
 using namespace llvm::coverage;
@@ -301,8 +306,9 @@
   if (!Visited.insert(File).second)
 continue;
 
-  // Do not map FileID's associated with system headers.
-  if (SM.isInSystemHeader(SM.getSpellingLoc(Loc)))
+  // Do not map FileID's associated with system headers unless collecting
+  // coverage from system headers is explicitly enabled.
+  if (!SystemHeadersCoverage && 
SM.isInSystemHeader(SM.getSpellingLoc(Loc)))
 continue;
 
   unsigned Depth = 0;
@@ -416,8 +422,10 @@
   SourceLocation LocStart = Region.getBeginLoc();
   assert(SM.getFileID(LocStart).isValid() && "region in invalid file");
 
-  // Ignore regions from system headers.
-  if (SM.isInSystemHeader(SM.getSpellingLoc(LocStart)))
+  // Ignore regions from system headers unless collecting coverage from
+  // system headers is explicitly enabled.
+  if (!SystemHeadersCoverage &&
+  SM.isInSystemHeader(SM.getSpellingLoc(LocStart)))
 continue;
 
   auto CovFileID = getCoverageFileID(LocStart);
@@ -1000,8 +1008,10 @@
   void VisitDecl(const Decl *D) {
 Stmt *Body = D->getBody();
 
-// Do not propagate region counts into system headers.
-if (Body && SM.isInSystemHeader(SM.getSpellingLoc(getStart(Body
+// Do not propagate region counts into system headers unless collecting
+// coverage from system headers is explicitly enabled.
+if (!SystemHeadersCoverage && Body &&
+SM.isInSystemHeader(SM.getSpellingLoc(getStart(Body
   return;
 
 // Do not visit the artificial children nodes of defaulted methods. The


Index: clang/test/CoverageMapping/system_macro.cpp
===
--- clang/test/CoverageMapping/system_macro.cpp
+++ clang/test/CoverageMapping/system_macro.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -mllvm -emptyline-comment-coverage=false -std=c++11 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name system_macro.cpp -o - %s | FileCheck %s
+// RUN: %clang_cc1 -mllvm -emptyline-comment-coverage=false -mllvm -system-headers-coverage -std=c++11 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name system_macro.cpp -o - %s | FileCheck %s
 
 #ifdef IS_SYSHEADER
 
@@ -13,8 +13,9 @@
 
 // CHECK-LABEL: doSomething
 void doSomething(int x) { // CHECK: File 0, [[@LINE]]:25 -> {{[0-9:]+}} = #0
-  Func(x);
+  Func(x); // CHECK: Expansion,File 0, [[@LINE]]:3 -> [[@LINE]]:7
   return;
+  // CHECK: Expansion,File 0, [[@LINE+1]]:3 -> [[@LINE+1]]:11
   SomeType *f; // CHECK: File 0, [[@LINE]]:11 -> {{[0-9:]+}} = 0
 }
 
Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -37,6 +37,11 @@
"disable it 

[PATCH] D143304: [Coverage] Map regions from system headers

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem created this revision.
Herald added a project: All.
gulfem requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Originally, the following commit removed mapping coverage regions for system 
headers:
https://github.com/llvm/llvm-project/commit/93205af066341a53733046894bd75c72c99566db

It might be viable and useful to collect coverage from system headers in some 
systems.
This patch adds --system-headers-coverage option (disabled by default) to enable
collecting coverage from system headers.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D143304

Files:
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/test/CoverageMapping/system_macro.cpp


Index: clang/test/CoverageMapping/system_macro.cpp
===
--- clang/test/CoverageMapping/system_macro.cpp
+++ clang/test/CoverageMapping/system_macro.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -mllvm -emptyline-comment-coverage=false -std=c++11 
-fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping 
-emit-llvm-only -main-file-name system_macro.cpp -o - %s | FileCheck %s
+// RUN: %clang_cc1 -mllvm -emptyline-comment-coverage=false -mllvm 
-system-headers-coverage -std=c++11 -fprofile-instrument=clang 
-fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name 
system_macro.cpp -o - %s | FileCheck %s
 
 #ifdef IS_SYSHEADER
 
@@ -13,8 +13,9 @@
 
 // CHECK-LABEL: doSomething
 void doSomething(int x) { // CHECK: File 0, [[@LINE]]:25 -> {{[0-9:]+}} = #0
-  Func(x);
+  Func(x); // CHECK: Expansion,File 0, [[@LINE]]:3 -> [[@LINE]]:7
   return;
+  // CHECK: Expansion,File 0, [[@LINE+1]]:3 -> [[@LINE+1]]:11
   SomeType *f; // CHECK: File 0, [[@LINE]]:11 -> {{[0-9:]+}} = 0
 }
 
Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -37,6 +37,11 @@
"disable it on test)"),
 llvm::cl::init(true), llvm::cl::Hidden);
 
+static llvm::cl::opt SystemHeadersCoverage(
+"system-headers-coverage",
+llvm::cl::desc("Enable collecting coverage from system headers"),
+llvm::cl::init(false), llvm::cl::Hidden);
+
 using namespace clang;
 using namespace CodeGen;
 using namespace llvm::coverage;
@@ -301,8 +306,9 @@
   if (!Visited.insert(File).second)
 continue;
 
-  // Do not map FileID's associated with system headers.
-  if (SM.isInSystemHeader(SM.getSpellingLoc(Loc)))
+  // Do not map FileID's associated with system headers unless collecting
+  // coverage from system headers is explicitly enabled.
+  if (!SystemHeadersCoverage && 
SM.isInSystemHeader(SM.getSpellingLoc(Loc)))
 continue;
 
   unsigned Depth = 0;
@@ -416,8 +422,10 @@
   SourceLocation LocStart = Region.getBeginLoc();
   assert(SM.getFileID(LocStart).isValid() && "region in invalid file");
 
-  // Ignore regions from system headers.
-  if (SM.isInSystemHeader(SM.getSpellingLoc(LocStart)))
+  // Ignore regions from system headers unless collecting coverage from
+  // system headers is explicitly enabled.
+  if (!SystemHeadersCoverage &&
+  SM.isInSystemHeader(SM.getSpellingLoc(LocStart)))
 continue;
 
   auto CovFileID = getCoverageFileID(LocStart);
@@ -1000,8 +1008,10 @@
   void VisitDecl(const Decl *D) {
 Stmt *Body = D->getBody();
 
-// Do not propagate region counts into system headers.
-if (Body && SM.isInSystemHeader(SM.getSpellingLoc(getStart(Body
+// Do not propagate region counts into system headers unless collecting
+// coverage from system headers is explicitly enabled.
+if (!SystemHeadersCoverage && Body &&
+SM.isInSystemHeader(SM.getSpellingLoc(getStart(Body
   return;
 
 // Do not visit the artificial children nodes of defaulted methods. The


Index: clang/test/CoverageMapping/system_macro.cpp
===
--- clang/test/CoverageMapping/system_macro.cpp
+++ clang/test/CoverageMapping/system_macro.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -mllvm -emptyline-comment-coverage=false -std=c++11 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name system_macro.cpp -o - %s | FileCheck %s
+// RUN: %clang_cc1 -mllvm -emptyline-comment-coverage=false -mllvm -system-headers-coverage -std=c++11 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name system_macro.cpp -o - %s | FileCheck %s
 
 #ifdef IS_SYSHEADER
 
@@ -13,8 +13,9 @@
 
 // CHECK-LABEL: doSomething
 void doSomething(int x) { // CHECK: File 0, [[@LINE]]:25 -> {{[0-9:]+}} = #0
-  Func(x);
+  Func(x); // CHECK: Expansion,File 0, [[@LINE]]:3 -> [[@LINE]]:7
   return;
+  // CHECK: Expansion,File 0, [[@LINE+1]]:3 -> [[@LINE+1]]:11
   SomeType 

[PATCH] D129755: Thread safety analysis: Support copy-elided production of scoped capabilities through arbitrary calls

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

In D129755#3844059 , @aaronpuchert 
wrote:

> In D129755#3843144 , @gulfem wrote:
>
>> We also started seeing `-Wthread-safety-precise` error in our Fuchsia code. 
>> https://luci-milo.appspot.com/ui/p/fuchsia/builders/ci/clang_toolchain.ci.core.x64-release/b8800959115965408001/overview
>> I'm trying to verify with our team whether it is a false positive, but I 
>> just wanted to give you heads up!
>
> This was a genuine bug which I believe to be fixed now (see the comments 
> below). Thanks for the report!
>
> I'd appreciate if you could test this, but it's not necessary as I could 
> reproduce the bug and the test seems to show it fixed.

I verified that it fixes the following error in our `zircon` kernel, and thanks 
for the fix!

  [89054/268252] CXX kernel_x64/obj/zircon/kernel/vm/vm.vm_cow_pages.cc.o
  FAILED: kernel_x64/obj/zircon/kernel/vm/vm.vm_cow_pages.cc.o
  ../../../recipe_cleanup/clang72pd81kv/bin/clang++ -MD -MF 
kernel_x64/obj/zircon/kernel/vm/vm.vm_cow_pages.cc.o.d -o 
kernel_x64/obj/zircon/kernel/vm/vm.vm_cow_pages.cc.o 
-D_LIBCPP_DISABLE_VISIBILITY...
  ../../zircon/kernel/vm/vm_cow_pages.cc:6400:3: error: calling function 
'~VmoCursor' requires holding mutex 'lock_' exclusively 
[-Werror,-Wthread-safety-precise]
}
^
  ../../zircon/kernel/vm/vm_cow_pages.cc:6400:3: note: found near match 
'cursor.lock_'
  ../../zircon/kernel/vm/vm_cow_pages.cc:6483:10: error: calling function 
'~VmoCursor' requires holding mutex 'lock_' exclusively 
[-Werror,-Wthread-safety-precise]
return total_pages_discarded;
   ^
  ../../zircon/kernel/vm/vm_cow_pages.cc:6483:10: note: found near match 
'cursor.lock_'


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129755

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

[PATCH] D129755: Thread safety analysis: Support copy-elided production of scoped capabilities through arbitrary calls

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

In D129755#3843206 , @aaronpuchert 
wrote:

> In D129755#3843144 , @gulfem wrote:
>
>> We also started seeing `-Wthread-safety-precise` error in our Fuchsia code. 
>> https://luci-milo.appspot.com/ui/p/fuchsia/builders/ci/clang_toolchain.ci.core.x64-release/b8800959115965408001/overview
>> I'm trying to verify with our team whether it is a false positive, but I 
>> just wanted to give you heads up!
>
> Both places have 
> 
>
>   Cursor cursor(DiscardableVmosLock::Get(), ...);
>   AssertHeld(cursor.lock_ref());
>
> At the end of the scope we get
>
>   error: calling function '~VmoCursor' requires holding mutex 'lock_' 
> exclusively [-Werror,-Wthread-safety-precise]
>   note: found near match 'cursor.lock_'
>
> Presumably `Cursor` is some kind of alias to `VmoCursor`, as we don't look at 
> base destructors yet. Since the code is not easily searchable for me, can you 
> look up the annotations on `DiscardableVmosLock::Get`, the constructor of 
> `Cursor`/`VmoCursor` being used here, `Cursor::lock_ref`, and `AssertHeld`?

https://cs.opensource.google/fuchsia/fuchsia/+/main:zircon/kernel/vm/vm_cow_pages.cc
It looks like `Cursor` is an alias to `VmoCursor`.
https://cs.opensource.google/fuchsia/fuchsia/+/main:zircon/kernel/vm/include/vm/vm_cow_pages.h

> Edit: the second error seems to be same that @hans was posting. That's a true 
> positive sadly, we didn't emit a warning previously by accident.

Yes, that is the same error.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129755

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

[PATCH] D129755: Thread safety analysis: Support copy-elided production of scoped capabilities through arbitrary calls

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

We also started seeing `-Wthread-safety-precise` error in our Fuchsia code. 
https://luci-milo.appspot.com/ui/p/fuchsia/builders/ci/clang_toolchain.ci.core.x64-release/b8800959115965408001/overview
I'm trying to verify with our team whether it is a false positive, but I just 
wanted to give you heads up!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129755

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

[PATCH] D135356: [Format] Fix crash when hitting eof while lexing JS template string

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

In D135356#3840428 , @sammccall wrote:

> Fixed by 4b53c00173163774d32125fbcae283a46a9a4b19 
>  I think.

It fixed the test error that we are seeing, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135356

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

[PATCH] D135356: [Format] Fix crash when hitting eof while lexing JS template string

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

We are seeing the same error reported above:

  FormatTests: clang/lib/Lex/Lexer.cpp:1151: SourceLocation 
clang::Lexer::getSourceLocation(const char *, unsigned int) const: Assertion 
`Loc >= BufferStart && Loc <= BufferEnd && "Location out of range for this 
buffer!"' failed.

https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8801040768684259873/overview


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135356

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

[PATCH] D131528: [Clang] Restrict non fixed enum to a value outside the range of the enumeration values warning to context requiring a constant expression

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

In D131528#3717509 , @shafik wrote:

> In D131528#3717141 , @gulfem wrote:
>
>> Can somebody please clarify this? Is the following code results in 
>> `undefined` behavior?
>>
>>   enum E1 {e11=-4, e12=4};
>>   E1 x2b = static_cast(8);
>>
>> `constexpr E1 x2 = static_cast(8)` seems like `undefined`, so 
>> `-Wenum-constexpr-conversion` is triggered.
>> We started seeing `-Wenum-constexpr-conversion` in our codebase after 
>> https://reviews.llvm.org/D131307, but we stopped seeing them after this 
>> review.
>> Is the first example that I show above still undefined, but 
>> `-Wenum-constexpr-conversion` is not going to warn in such cases?
>
> Yes, this is indeed undefined behavior. The initial patch effected a lot of 
> users and several third party packages and so I am trying to narrow the 
> impact by restricting the error to those cases that are constant expression 
> contexts only.
>
> So you may indeed less diagnostics now.

Thanks for your response @shafik. This is something temporary to ease the 
transition, but we will enable `-Wenum-constexpr-conversion` for non-const 
expressions at some point, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131528

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

[PATCH] D131528: [Clang] Restrict non fixed enum to a value outside the range of the enumeration values warning to context requiring a constant expression

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

Can somebody please clarify this? Is the following code results `undefined` 
behavior?

  enum E1 {e11=-4, e12=4};
  E1 x2b = static_cast(8);

`constexpr E1 x2 = static_cast(8)` seems like `undefined`, so 
`-Wenum-constexpr-conversion` is triggered.
We started seeing `-Wenum-constexpr-conversion` in our codebase after 
https://reviews.llvm.org/D131307, but we stopped seeing them after this review.
Is the first example that I show above still undefined, but 
`-Wenum-constexpr-conversion` is not going to warn in such cases?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131528

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

[PATCH] D90275: [clang][IR] Add support for leaf attribute

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

Added the description in https://reviews.llvm.org/D131628


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90275

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

[PATCH] D90275: [clang][IR] Add support for leaf attribute

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

In D90275#3712991 , @nlopes wrote:

> In D90275#3712969 , @gulfem wrote:
>
>> In D90275#3712880 , @nlopes wrote:
>>
>>> In D90275#3707793 , @nlopes wrote:
>>>
 In D90275#3671983 , @gulfem wrote:

> In D90275#3671019 , @nlopes 
> wrote:
>
>> Could you please add a description of this attribute to LangRef?
>> We need the semantics of this.
>>
>> Thank you!
>
> Sure, I'll do that!

 ping?
>>>
>>> Since the patch author has been ignoring my ask (and is active), and since 
>>> documenting IR features is of paramount importance, I suggest we revert 
>>> this patch. I think 3 weeks to document something is sufficient, and the 
>>> author didn't provide an ETA.
>>> I would like to ask everyone to reject all future patches that change the 
>>> IR and don't include documentation, as experience says that we'll get 
>>> miscompilations due to different people having different ideas of the 
>>> semantics.
>>>
>>> TL;DR: I'll revert this patch on Friday if no one chimes in. Thank you!
>>
>> I'm very sorry and I was not ignoring your ask. I'll do it now.
>
> Thanks!
> I'm fine with a reasonable ETA, but we really need all IR features documented 
> to avoid bugs.

The urgency was not clear to me at the beginning, and I already added this to 
my list. 
I did not have a chance to respond to your comment this week because of 
traveling.
But again, I did not mean to ignore this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90275

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

[PATCH] D90275: [clang][IR] Add support for leaf attribute

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

In D90275#3712880 , @nlopes wrote:

> In D90275#3707793 , @nlopes wrote:
>
>> In D90275#3671983 , @gulfem wrote:
>>
>>> In D90275#3671019 , @nlopes wrote:
>>>
 Could you please add a description of this attribute to LangRef?
 We need the semantics of this.

 Thank you!
>>>
>>> Sure, I'll do that!
>>
>> ping?
>
> Since the patch author has been ignoring my ask (and is active), and since 
> documenting IR features is of paramount importance, I suggest we revert this 
> patch. I think 3 weeks to document something is sufficient, and the author 
> didn't provide an ETA.
> I would like to ask everyone to reject all future patches that change the IR 
> and don't include documentation, as experience says that we'll get 
> miscompilations due to different people having different ideas of the 
> semantics.
>
> TL;DR: I'll revert this patch on Friday if no one chimes in. Thank you!

I'm very sorry and I was not ignoring your ask. I'll do it now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90275

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

[PATCH] D90275: [clang][IR] Add support for leaf attribute

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

In D90275#3671019 , @nlopes wrote:

> Could you please add a description of this attribute to LangRef?
> We need the semantics of this.
>
> Thank you!

Sure, I'll do that!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90275

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

[PATCH] D126586: [InstrProf][WIP] Implement boolean counters in coverage

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

In D126586#3646197 , @davidxl wrote:

> For coverage mode, why using 'incrementProfileCounter'?  Should it be set to 
> '1' instead?  Also is the 'or' expression needed? The expression can be 
> folded to either zero or 1.

@davidxl I used `incrementProfileCounter()`, but it emits 
`llvm.instrprof.cover` intrinsic instead of `llvm.instrprof.increment` in 
`emitCounterIncrement()`. If it confusing, I can move the boolean counter 
handling into another function and name it like `setProfileCounter`, 
`setProfileCovered` or something like that.

I have one concern that I would like to hear your recommendation: 
`incrementProfileCounter()` is embedded into `Clang CodeGen` for various `AST` 
nodes. So, even for implementing a simple prototype to collect some numbers 
will require me doing invasive changes to `Clang CodeGen`. These are going to 
be small changes to several AST nodes. I have only done it for some frequently 
used `AST` nodes, but there are many more of them. Can you think of a better 
way of designing that? Based on the current implementation of instrumentation, 
I could not find a less invasive way of implementing boolean counters. If 
that's the only way to go, shall we land them in stages after the reviews?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126586

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

[PATCH] D126586: [InstrProf][WIP] Implement boolean counters in coverage

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

@davidxl and @vsk,
This is a WIP prototype implementation, and I would love to hear your early 
feedback on using 1-byte counters for coverage idea if possible.
I'll continue extending the prototype, but that would be great to learn if 
there is anything fundamentally wrong or missing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126586

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

[PATCH] D126586: [InstrProf][WIP] Implement boolean counters in coverage

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem updated this revision to Diff 434224.
gulfem added a comment.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Describe the high-level changes to adding and subtracting counters.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126586

Files:
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenPGO.cpp
  clang/lib/CodeGen/CodeGenPGO.h
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/test/CoverageMapping/boolean-counters.cpp
  llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
  llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
  llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp

Index: llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
===
--- llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
+++ llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
@@ -197,7 +197,8 @@
   Tag -= Counter::Expression;
   switch (Tag) {
   case CounterExpression::Subtract:
-  case CounterExpression::Add: {
+  case CounterExpression::Add:
+  case CounterExpression::Or: {
 auto ID = Value >> Counter::EncodingTagBits;
 if (ID >= Expressions.size())
   return make_error(coveragemap_error::malformed);
Index: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
===
--- llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
+++ llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
@@ -134,6 +134,14 @@
   return Simplify ? simplify(Cnt) : Cnt;
 }
 
+Counter CounterExpressionBuilder::orCounters(Counter LHS, Counter RHS) {
+  if (LHS.getKind() == Counter::Zero)
+return Counter::getCounter(RHS.getCounterID());
+  if (RHS.getKind() == Counter::Zero)
+return Counter::getCounter(LHS.getCounterID());
+  return get(CounterExpression(CounterExpression::Or, LHS, RHS));
+}
+
 void CounterMappingContext::dump(const Counter , raw_ostream ) const {
   switch (C.getKind()) {
   case Counter::Zero:
@@ -148,7 +156,10 @@
 const auto  = Expressions[C.getExpressionID()];
 OS << '(';
 dump(E.LHS, OS);
-OS << (E.Kind == CounterExpression::Subtract ? " - " : " + ");
+if (E.Kind == CounterExpression::Or)
+  OS << " || ";
+else
+  OS << (E.Kind == CounterExpression::Subtract ? " - " : " + ");
 dump(E.RHS, OS);
 OS << ')';
 break;
@@ -182,6 +193,9 @@
 Expected RHS = evaluate(E.RHS);
 if (!RHS)
   return RHS;
+if (E.Kind == CounterExpression::Or)
+  return *RHS || *LHS;
+
 return E.Kind == CounterExpression::Subtract ? *LHS - *RHS : *LHS + *RHS;
   }
   }
Index: llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
===
--- llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
+++ llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
@@ -93,8 +93,8 @@
   /// The CounterExpression kind (Add or Subtract) is encoded in bit 0 next to
   /// the CounterKind. This means CounterKind has to leave bit 0 free.
   enum CounterKind { Zero, CounterValueReference, Expression };
-  static const unsigned EncodingTagBits = 2;
-  static const unsigned EncodingTagMask = 0x3;
+  static const unsigned EncodingTagBits = 3;
+  static const unsigned EncodingTagMask = 0x7;
   static const unsigned EncodingCounterTagAndExpansionRegionTagBits =
   EncodingTagBits + 1;
 
@@ -147,7 +147,7 @@
 /// A Counter expression is a value that represents an arithmetic operation
 /// with two counters.
 struct CounterExpression {
-  enum ExprKind { Subtract, Add };
+  enum ExprKind { Subtract, Add, Or };
   ExprKind Kind;
   Counter LHS, RHS;
 
@@ -200,6 +200,9 @@
   /// Return a counter that represents the expression that subtracts RHS from
   /// LHS.
   Counter subtract(Counter LHS, Counter RHS, bool Simplify = true);
+
+  /// Return a counter that represents the expression RHS || LHS
+  Counter orCounters(Counter LHS, Counter RHS);
 };
 
 using LineColPair = std::pair;
Index: clang/test/CoverageMapping/boolean-counters.cpp
===
--- /dev/null
+++ clang/test/CoverageMapping/boolean-counters.cpp
@@ -0,0 +1,112 @@
+// RUN: %clang_cc1 -mllvm -emptyline-comment-coverage=false -mllvm -enable-boolean-counters=true -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name return.c %s | FileCheck %s
+
+// CHECK: _Z6testIfi
+int testIf(int x) { // CHECK-NEXT: File 0, [[@LINE]]:19 -> [[@LINE+10]]:2 = #0
+// CHECK-NEXT: File 0, [[@LINE+5]]:7 -> [[@LINE+5]]:13 = #0
+// CHECK-NEXT: Gap,File 0, [[@LINE+4]]:14 -> [[@LINE+5]]:5 = #2
+// CHECK-NEXT: File 0, [[@LINE+4]]:5 -> [[@LINE+4]]:16 = #2
+// CHECK-NEXT: File 0, [[@LINE+5]]:3 -> [[@LINE+5]]:16 = #1
+  int result = 0;
+  

[PATCH] D127040: [InstrProf][WIP] Implement boolean counters in coverage

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem created this revision.
Herald added subscribers: ellis, wenlei, hiraditya.
Herald added a project: All.
gulfem requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

This patch inserts 1-byte boolean counters instead of an 8-byte counters
into llvm profiles for source-based code coverage. The origial idea was
proposed as "block-cov" for PGO, and this patch reuses that idea for coverage.
https://groups.google.com/g/llvm-dev/c/r03Z6JoN7d4

This is a WIP prototype implementation which only implements boolean
counters for only a handful of control-flow statements (while, for, if, switch).
I will continue to extend it for other control-flow statements.

The current 8-byte counters mechanism add counters to minimal regions,
and infer the counters in the remaining regions via adding or
subtracting counters. Whenever there is control-flow merge, it adds
the counters from all the incoming regions. When we use boolean counters,
we can use a new counter expression called `or` instead of an and for such
cases. Therefore, this patch adds an Or kind into CounterExpression.

Similarly, 8-byte counters mechanism infers the counters in some regions
by subtracting counters. For example, it infers the counter in the `if.else`
region by subtracting the counters between `if.entry` and `if.then` regions
in an if statement. However, we are not going to be able to infer counters
by subtracting two execution counts when using boolean counters.
Therefore, we need to insert additional counters for the cases where we
need to subtract counters.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D127040

Files:
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenPGO.cpp
  clang/lib/CodeGen/CodeGenPGO.h
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/test/CoverageMapping/boolean-counters.cpp
  llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
  llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
  llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp

Index: llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
===
--- llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
+++ llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
@@ -197,7 +197,8 @@
   Tag -= Counter::Expression;
   switch (Tag) {
   case CounterExpression::Subtract:
-  case CounterExpression::Add: {
+  case CounterExpression::Add:
+  case CounterExpression::Or: {
 auto ID = Value >> Counter::EncodingTagBits;
 if (ID >= Expressions.size())
   return make_error(coveragemap_error::malformed);
Index: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
===
--- llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
+++ llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
@@ -134,6 +134,14 @@
   return Simplify ? simplify(Cnt) : Cnt;
 }
 
+Counter CounterExpressionBuilder::orCounters(Counter LHS, Counter RHS) {
+  if (LHS.getKind() == Counter::Zero)
+return Counter::getCounter(RHS.getCounterID());
+  if (RHS.getKind() == Counter::Zero)
+return Counter::getCounter(LHS.getCounterID());
+  return get(CounterExpression(CounterExpression::Or, LHS, RHS));
+}
+
 void CounterMappingContext::dump(const Counter , raw_ostream ) const {
   switch (C.getKind()) {
   case Counter::Zero:
@@ -148,7 +156,10 @@
 const auto  = Expressions[C.getExpressionID()];
 OS << '(';
 dump(E.LHS, OS);
-OS << (E.Kind == CounterExpression::Subtract ? " - " : " + ");
+if (E.Kind == CounterExpression::Or)
+  OS << " || ";
+else
+  OS << (E.Kind == CounterExpression::Subtract ? " - " : " + ");
 dump(E.RHS, OS);
 OS << ')';
 break;
@@ -182,6 +193,9 @@
 Expected RHS = evaluate(E.RHS);
 if (!RHS)
   return RHS;
+if (E.Kind == CounterExpression::Or)
+  return *RHS || *LHS;
+
 return E.Kind == CounterExpression::Subtract ? *LHS - *RHS : *LHS + *RHS;
   }
   }
Index: llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
===
--- llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
+++ llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
@@ -93,8 +93,8 @@
   /// The CounterExpression kind (Add or Subtract) is encoded in bit 0 next to
   /// the CounterKind. This means CounterKind has to leave bit 0 free.
   enum CounterKind { Zero, CounterValueReference, Expression };
-  static const unsigned EncodingTagBits = 2;
-  static const unsigned EncodingTagMask = 0x3;
+  static const unsigned EncodingTagBits = 3;
+  static const unsigned EncodingTagMask = 0x7;
   static const unsigned EncodingCounterTagAndExpansionRegionTagBits =
   EncodingTagBits + 1;
 
@@ -147,7 +147,7 @@
 /// A Counter expression is a value that represents an arithmetic operation
 /// with two counters.
 struct 

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

> Then pipe that to a file (note the -E I added at the end).  You should get a 
> file that looks like some slightly-wonky C++ code.

I got the following output after running it via `-E`.

  
/usr/local/google/home/gulfem/llvm-clean/llvm-project/libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_range.compile.pass.cpp:15:10:
 fatal error: 'memory' file not found
  #include 
   ^~~~
  # 1 
"/usr/local/google/home/gulfem/llvm-clean/llvm-project/libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_range.compile.pass.cpp"
  # 1 "" 1
  # 1 "" 3
  # 434 "" 3
  # 1 "" 1
  # 1 "" 2
  # 1 
"/usr/local/google/home/gulfem/llvm-clean/llvm-project/libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_range.compile.pass.cpp"
 2
  # 21 
"/usr/local/google/home/gulfem/llvm-clean/llvm-project/libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_range.compile.pass.cpp"
  template 
  struct ForwardProxyIterator {
using value_type = int;
using difference_type = int;
ForwardProxyIterator& operator++();
ForwardProxyIterator operator++(int);
bool operator==(const ForwardProxyIterator&) const;
  
int operator*() const;
  };
  
  
static_assert(std::ranges::__nothrow_forward_range>);
  
static_assert(!std::ranges::__nothrow_forward_range>);
  static_assert(std::ranges::forward_range>);
  
static_assert(!std::ranges::__nothrow_forward_range>);
  
  constexpr bool forward_subsumes_input(std::ranges::__nothrow_forward_range 
auto&&) {
return true;
  }
  constexpr bool forward_subsumes_input(std::ranges::__nothrow_input_range 
auto&&);
  
  static_assert(forward_subsumes_input("foo"));
  1 error generated.

Would that be enough?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

In D119544#3486241 , @erichkeane 
wrote:

> Ah shucks... Thanks for the heads up.  Is there any chance to get you to get 
> me a pre-processed version of this failure to play with?  I've not had luck 
> compiling/running libc++ tests in the past.

Sure, I'll try to help. What exactly do you need?
Is it possible to revert the change while you are working on the fix?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119544

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

[PATCH] D119544: Deferred Concept Instantiation Implementation

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

We started seeing several test failures after this commit:
https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8815265760499763361/overview

One example is `nothrow_forward_range.compile.pass.cpp`.

  Script:
  --
  : 'COMPILED WITH';  /b/s/w/ir/x/w/staging/llvm_build/./bin/clang++ 
/b/s/w/ir/x/w/llvm-llvm-project/libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_range.compile.pass.cpp
  --target=x86_64-unknown-linux-gnu -nostdinc++ -I 
/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1 -I 
/b/s/w/ir/x/w/staging/llvm_build/include/x86_64-unknown-linux-gnu/c++/v1 -I 
/b/s/w/ir/x/w/llvm-llvm-project/libcxx/test/support -std=c++2b -Werror -Wall 
-Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes 
-Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type 
-Wno-atomic-alignment -Wno-user-defined-literals -Wsign-compare 
-Wunused-variable -Wunused-parameter -Wunreachable-code 
-Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER 
-D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety 
-Wuser-defined-warnings  -fsyntax-only
  --
  Exit Code: 1
  
  Command Output (stdout):
  --
  $ ":" "COMPILED WITH"
  $ "/b/s/w/ir/x/w/staging/llvm_build/./bin/clang++" 
"/b/s/w/ir/x/w/llvm-llvm-project/libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_range.compile.pass.cpp"
 "--target=x86_64-unknown-linux-gnu" "-nostdinc++" "-I" 
"/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1" "-I" 
"/b/s/w/ir/x/w/staging/llvm_build/include/x86_64-unknown-linux-gnu/c++/v1" "-I" 
"/b/s/w/ir/x/w/llvm-llvm-project/libcxx/test/support" "-std=c++2b" "-Werror" 
"-Wall" "-Wextra" "-Wshadow" "-Wundef" "-Wno-unused-command-line-argument" 
"-Wno-attributes" "-Wno-pessimizing-move" "-Wno-c++11-extensions" 
"-Wno-noexcept-type" "-Wno-atomic-alignment" "-Wno-user-defined-literals" 
"-Wsign-compare" "-Wunused-variable" "-Wunused-parameter" "-Wunreachable-code" 
"-Wno-unused-local-typedef" "-D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER" 
"-D_LIBCPP_DISABLE_AVAILABILITY" "-fcoroutines-ts" "-Werror=thread-safety" 
"-Wuser-defined-warnings" "-fsyntax-only"
  # command stderr:
  In file included from 
/b/s/w/ir/x/w/llvm-llvm-project/libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_forward_range.compile.pass.cpp:18:
  In file included from 
/b/s/w/ir/x/w/llvm-llvm-project/libcxx/test/support/test_range.h:12:
  In file included from 
/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/ranges:278:
  In file included from 
/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/__ranges/all.h:18:
  
/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/__ranges/range_adaptor.h:54:37: 
error: redefinition of 'operator|'
  friend constexpr decltype(auto) operator|(_View&& __view, _Closure&& 
__closure)
  ^
  
/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/__ranges/common_view.h:107:17: 
note: in instantiation of template class 
'std::__range_adaptor_closure' requested 
here
struct __fn : __range_adaptor_closure<__fn> {
  ^
  
/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/__ranges/range_adaptor.h:54:37: 
note: previous definition is here
  friend constexpr decltype(auto) operator|(_View&& __view, _Closure&& 
__closure)
  ^
  
/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/__ranges/range_adaptor.h:63:27: 
error: redefinition of 'operator|'
  friend constexpr auto operator|(_Closure&& __c1, _OtherClosure&& __c2)
^
  
/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/__ranges/range_adaptor.h:63:27: 
note: previous definition is here
  
/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1/__ranges/range_adaptor.h:54:37: 
error: redefinition of 'operator|'
  friend constexpr decltype(auto) operator|(_View&& __view, _Closure&& 
__closure)
  --


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119544

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

[PATCH] D122336: [InstrProfiling] No runtime hook for unused funcs

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

@davidxl,
I added comments in 
https://github.com/llvm/llvm-project/commit/ead8586645f54de16cfc6fa26028d818efc425f2

@MaskRay,
I followed the style guide for nested if statements in 
https://github.com/llvm/llvm-project/commit/ead8586645f54de16cfc6fa26028d818efc425f2


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122336

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

[PATCH] D122336: [InstrProfiling] No runtime hook for unused funcs

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added inline comments.



Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:569
+  if (!containsProfilingIntrinsics(M)) {
+if (!CoverageNamesVar || !NeedsRuntimeHook) {
+  return MadeChange;

MaskRay wrote:
> https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements
> 
> That said, nested if is usually written as a single if with `&&`
Thanks for the tip @MaskRay, and I'm going to fix that in a following patch. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122336

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

[PATCH] D122336: [InstrProfiling] No runtime hook for unused funcs

[Gulfem Savrun Yeniceri 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 rGc7f91e227a79: [InstrProfiling] No runtime hook for unused 
funcs (authored by gulfem).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122336

Files:
  clang/test/CoverageMapping/unused_function_no_runtime_hook.cpp
  llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp


Index: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
===
--- llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
+++ llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
@@ -558,16 +558,18 @@
   TT = Triple(M.getTargetTriple());
 
   bool MadeChange = false;
-
-  // Emit the runtime hook even if no counters are present.
-  if (needsRuntimeHookUnconditionally(TT))
+  bool NeedsRuntimeHook = needsRuntimeHookUnconditionally(TT);
+  if (NeedsRuntimeHook)
 MadeChange = emitRuntimeHook();
 
   // Improve compile time by avoiding linear scans when there is no work.
   GlobalVariable *CoverageNamesVar =
   M.getNamedGlobal(getCoverageUnusedNamesVarName());
-  if (!containsProfilingIntrinsics(M) && !CoverageNamesVar)
-return MadeChange;
+  if (!containsProfilingIntrinsics(M)) {
+if (!CoverageNamesVar || !NeedsRuntimeHook) {
+  return MadeChange;
+}
+  }
 
   // We did not know how many value sites there would be inside
   // the instrumented function. This is counting the number of instrumented
Index: clang/test/CoverageMapping/unused_function_no_runtime_hook.cpp
===
--- /dev/null
+++ clang/test/CoverageMapping/unused_function_no_runtime_hook.cpp
@@ -0,0 +1,6 @@
+// RUN: %clang -target x86_64-unknown-fuchsia -fprofile-instr-generate 
-fcoverage-mapping -emit-llvm -S %s -o - | FileCheck %s
+
+// CHECK-NOT: @__llvm_profile_runtime
+static int f0() {
+  return 100;
+}


Index: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
===
--- llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
+++ llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
@@ -558,16 +558,18 @@
   TT = Triple(M.getTargetTriple());
 
   bool MadeChange = false;
-
-  // Emit the runtime hook even if no counters are present.
-  if (needsRuntimeHookUnconditionally(TT))
+  bool NeedsRuntimeHook = needsRuntimeHookUnconditionally(TT);
+  if (NeedsRuntimeHook)
 MadeChange = emitRuntimeHook();
 
   // Improve compile time by avoiding linear scans when there is no work.
   GlobalVariable *CoverageNamesVar =
   M.getNamedGlobal(getCoverageUnusedNamesVarName());
-  if (!containsProfilingIntrinsics(M) && !CoverageNamesVar)
-return MadeChange;
+  if (!containsProfilingIntrinsics(M)) {
+if (!CoverageNamesVar || !NeedsRuntimeHook) {
+  return MadeChange;
+}
+  }
 
   // We did not know how many value sites there would be inside
   // the instrumented function. This is counting the number of instrumented
Index: clang/test/CoverageMapping/unused_function_no_runtime_hook.cpp
===
--- /dev/null
+++ clang/test/CoverageMapping/unused_function_no_runtime_hook.cpp
@@ -0,0 +1,6 @@
+// RUN: %clang -target x86_64-unknown-fuchsia -fprofile-instr-generate -fcoverage-mapping -emit-llvm -S %s -o - | FileCheck %s
+
+// CHECK-NOT: @__llvm_profile_runtime
+static int f0() {
+  return 100;
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122336: [InstrProfiling] No runtime hook for unused funcs

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

In D122336#3406843 , @vsk wrote:

> So long as it doesn't change behavior expected by tapi on Darwin, I think 
> it's OK. I doubt any other platforms are similarly affected.

I don't think it should impact TAPI because it relies on 
`needsRuntimeHookUnconditionally` function, which only returns false for 
`Fuchsia`.
So, this patch only affects the behavior of pulling in profile runtime for 
unused functions in `Fuchsia`.

  bool needsRuntimeHookUnconditionally(const Triple ) {
// On Fuchsia, we only need runtime hook if any counters are present.
if (TT.isOSFuchsia())
  return false;
  
return true;
  }




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122336

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

[PATCH] D122336: [InstrProfiling] No runtime hook for unused funcs

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

In D122336#3406186 , @vsk wrote:

> Sorry for ay delayed replies - I've switched teams at Apple and find it 
> difficult to keep up with llvm reviews.
>
>> it's my understanding is that we might be generating coverage record for 
>> unused functions for TAPI.
>
> Coverage function records are emitted for unused functions because the 
> tooling needs to know which file/line ranges require a "0" execution count.

Thanks for the background @vsk. That means that we need to generate a coverage 
record even for unused functions.
Do you have any objection to the current solution (not pulling in profile 
runtime for such cases)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122336

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

[PATCH] D122336: [InstrProfiling] No runtime hook for unused funcs

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

In D122336#3405835 , @davidxl wrote:

> Should this be fixed in Clang (not generating coverage record)? Adding the 
> logic here is a little confusing.

@davidxl,
I originally did that (not generating coverage records) because I think it is a 
cleaner way.
But, after reading https://reviews.llvm.org/D43794, it's my understanding is 
that we might be generating coverage record for unused functions for TAPI.
@vsk can clarify that. If there is no such restriction anymore, I can change 
that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122336

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

[PATCH] D122336: [InstrProfiling] No runtime hook for unused funcs

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem updated this revision to Diff 417945.
gulfem added a comment.

Addressed phosek's comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122336

Files:
  clang/test/CoverageMapping/unused_function_no_runtime_hook.cpp
  llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp


Index: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
===
--- llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
+++ llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
@@ -558,16 +558,18 @@
   TT = Triple(M.getTargetTriple());
 
   bool MadeChange = false;
-
-  // Emit the runtime hook even if no counters are present.
-  if (needsRuntimeHookUnconditionally(TT))
+  bool NeedsRuntimeHook = needsRuntimeHookUnconditionally(TT);
+  if (NeedsRuntimeHook)
 MadeChange = emitRuntimeHook();
 
   // Improve compile time by avoiding linear scans when there is no work.
   GlobalVariable *CoverageNamesVar =
   M.getNamedGlobal(getCoverageUnusedNamesVarName());
-  if (!containsProfilingIntrinsics(M) && !CoverageNamesVar)
-return MadeChange;
+  if (!containsProfilingIntrinsics(M)) {
+if (!CoverageNamesVar || !NeedsRuntimeHook) {
+  return MadeChange;
+}
+  }
 
   // We did not know how many value sites there would be inside
   // the instrumented function. This is counting the number of instrumented
Index: clang/test/CoverageMapping/unused_function_no_runtime_hook.cpp
===
--- /dev/null
+++ clang/test/CoverageMapping/unused_function_no_runtime_hook.cpp
@@ -0,0 +1,6 @@
+// RUN: %clang -target x86_64-unknown-fuchsia -fprofile-instr-generate 
-fcoverage-mapping -emit-llvm -S %s -o - | FileCheck %s
+
+// CHECK-NOT: @__llvm_profile_runtime
+static int f0() {
+  return 100;
+}


Index: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
===
--- llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
+++ llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
@@ -558,16 +558,18 @@
   TT = Triple(M.getTargetTriple());
 
   bool MadeChange = false;
-
-  // Emit the runtime hook even if no counters are present.
-  if (needsRuntimeHookUnconditionally(TT))
+  bool NeedsRuntimeHook = needsRuntimeHookUnconditionally(TT);
+  if (NeedsRuntimeHook)
 MadeChange = emitRuntimeHook();
 
   // Improve compile time by avoiding linear scans when there is no work.
   GlobalVariable *CoverageNamesVar =
   M.getNamedGlobal(getCoverageUnusedNamesVarName());
-  if (!containsProfilingIntrinsics(M) && !CoverageNamesVar)
-return MadeChange;
+  if (!containsProfilingIntrinsics(M)) {
+if (!CoverageNamesVar || !NeedsRuntimeHook) {
+  return MadeChange;
+}
+  }
 
   // We did not know how many value sites there would be inside
   // the instrumented function. This is counting the number of instrumented
Index: clang/test/CoverageMapping/unused_function_no_runtime_hook.cpp
===
--- /dev/null
+++ clang/test/CoverageMapping/unused_function_no_runtime_hook.cpp
@@ -0,0 +1,6 @@
+// RUN: %clang -target x86_64-unknown-fuchsia -fprofile-instr-generate -fcoverage-mapping -emit-llvm -S %s -o - | FileCheck %s
+
+// CHECK-NOT: @__llvm_profile_runtime
+static int f0() {
+  return 100;
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D121812: [clang][deps] NFC: De-duplicate clang-cl tests

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

In D121812#3405060 , @jansvoboda11 
wrote:

> Sorry for the breakage and thanks for reporting this! This is a real bug 
> uncovered by your build using `/opt`. I have a fix here: D122385 
> .

Thanks for the fix.  Could it be possible to land the fix asap?
If not, could you please revert this patch, and reland with the fix?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121812

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

[PATCH] D121812: [clang][deps] NFC: De-duplicate clang-cl tests

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

Newly added tests `ClangScanDeps/cl-output.c` and `ClangScanDeps/cl-xclang.c` 
started failing in our Mac toolchain builds:
https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-mac-x64/b881168677577537/overview
Here's the link to the log:
https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket/881168677577537/+/u/clang/test/stdout

The failure is as the following:

  FAIL: Clang :: ClangScanDeps/cl-xclang.c (2170 of 30232)
   TEST 'Clang :: ClangScanDeps/cl-xclang.c' FAILED 

  Script:
  --
  : 'RUN: at line 4';   rm -rf 
/opt/s/w/ir/x/w/staging/llvm_build/tools/clang/test/ClangScanDeps/Output/cl-xclang.c.tmp
  : 'RUN: at line 5';   split-file 
/opt/s/w/ir/x/w/llvm-llvm-project/clang/test/ClangScanDeps/cl-xclang.c 
/opt/s/w/ir/x/w/staging/llvm_build/tools/clang/test/ClangScanDeps/Output/cl-xclang.c.tmp
  : 'RUN: at line 16';   sed -e 
"s|DIR|/opt/s/w/ir/x/w/staging/llvm_build/tools/clang/test/ClangScanDeps/Output/cl-xclang.c.tmp|g"
 
/opt/s/w/ir/x/w/staging/llvm_build/tools/clang/test/ClangScanDeps/Output/cl-xclang.c.tmp/cdb.json.template
 > 
/opt/s/w/ir/x/w/staging/llvm_build/tools/clang/test/ClangScanDeps/Output/cl-xclang.c.tmp/cdb.json
  : 'RUN: at line 17';   /opt/s/w/ir/x/w/staging/llvm_build/bin/clang-scan-deps 
-compilation-database 
/opt/s/w/ir/x/w/staging/llvm_build/tools/clang/test/ClangScanDeps/Output/cl-xclang.c.tmp/cdb.json
 > 
/opt/s/w/ir/x/w/staging/llvm_build/tools/clang/test/ClangScanDeps/Output/cl-xclang.c.tmp/result.d
  : 'RUN: at line 18';   cat 
/opt/s/w/ir/x/w/staging/llvm_build/tools/clang/test/ClangScanDeps/Output/cl-xclang.c.tmp/result.d
 | sed 's:\?:/:g' | /opt/s/w/ir/x/w/staging/llvm_build/bin/FileCheck 
/opt/s/w/ir/x/w/llvm-llvm-project/clang/test/ClangScanDeps/cl-xclang.c 
-DPREFIX=/opt/s/w/ir/x/w/staging/llvm_build/tools/clang/test/ClangScanDeps/Output/cl-xclang.c.tmp
  --
  Exit Code: 1
  
  Command Output (stderr):
  --
  /opt/s/w/ir/x/w/llvm-llvm-project/clang/test/ClangScanDeps/cl-xclang.c:19:11: 
error: CHECK: expected string not found in input
  // CHECK: [[PREFIX]]/test.o:
^
  :1:1: note: scanning from here
  
pt/s/w/ir/x/w/staging/llvm_build/tools/clang/test/ClangScanDeps/Output/cl-xclang.c.tmp/test.o:
 \
  ^
  :1:1: note: with "PREFIX" equal to 
"/opt/s/w/ir/x/w/staging/llvm_build/tools/clang/test/ClangScanDeps/Output/cl-xclang\\.c\\.tmp"
  
pt/s/w/ir/x/w/staging/llvm_build/tools/clang/test/ClangScanDeps/Output/cl-xclang.c.tmp/test.o:
 \
  ^
  :1:86: note: possible intended match here
  
pt/s/w/ir/x/w/staging/llvm_build/tools/clang/test/ClangScanDeps/Output/cl-xclang.c.tmp/test.o:
 \

   ^
  
  Input file: 
  Check file: 
/opt/s/w/ir/x/w/llvm-llvm-project/clang/test/ClangScanDeps/cl-xclang.c
  
  -dump-input=help explains the following input dump.
  
  Input was:
  <<
  1: 
pt/s/w/ir/x/w/staging/llvm_build/tools/clang/test/ClangScanDeps/Output/cl-xclang.c.tmp/test.o:
 \ 
  check:19'0 
X
 error: no match found
  check:19'1
   with "PREFIX" equal to 
"/opt/s/w/ir/x/w/staging/llvm_build/tools/clang/test/ClangScanDeps/Output/cl-xclang\\.c\\.tmp"
  check:19'2
  ?possible intended match
  2:  
/opt/s/w/ir/x/w/staging/llvm_build/tools/clang/test/ClangScanDeps/Output/cl-xclang.c.tmp/test.c
 
  check:19'0 
~
  >>
  
  --
  
  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121812

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

[PATCH] D122336: [InstrProfiling] No runtime hook for unused funcs

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem created this revision.
Herald added subscribers: ellis, abrachet, phosek, hiraditya.
Herald added a project: All.
gulfem requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

CoverageMappingModuleGen generates a coverage mapping record
even for unused functions with internal linkage, e.g.
static int foo() { return 100; }
Clang frontend eliminates such functions, but InstrProfiling pass
still pulls in profile runtime since there is a coverage record.
Fuchsia uses runtime counter relocation, and pulling in profile
runtime for unused functions causes a linker error:
undefined hidden symbol: __llvm_profile_counter_bias.
Since 389dc94d4be7 
, we do 
not hook profile runtime for the binaries
that none of its translation units have been instrumented in Fuchsia.
This patch extends that for the instrumented binaries that
consist of only unused functions.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D122336

Files:
  clang/test/CoverageMapping/unused_function_no_runtime_hook.cpp
  llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp


Index: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
===
--- llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
+++ llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
@@ -558,16 +558,20 @@
   TT = Triple(M.getTargetTriple());
 
   bool MadeChange = false;
-
-  // Emit the runtime hook even if no counters are present.
-  if (needsRuntimeHookUnconditionally(TT))
+  bool NeedsRuntimeHook = needsRuntimeHookUnconditionally(TT);
+  if (NeedsRuntimeHook)
 MadeChange = emitRuntimeHook();
 
   // Improve compile time by avoiding linear scans when there is no work.
   GlobalVariable *CoverageNamesVar =
   M.getNamedGlobal(getCoverageUnusedNamesVarName());
-  if (!containsProfilingIntrinsics(M) && !CoverageNamesVar)
-return MadeChange;
+  if (!containsProfilingIntrinsics(M)) {
+if (!CoverageNamesVar) {
+  return MadeChange;
+} else if (!NeedsRuntimeHook) {
+  return MadeChange;
+}
+  }
 
   // We did not know how many value sites there would be inside
   // the instrumented function. This is counting the number of instrumented
Index: clang/test/CoverageMapping/unused_function_no_runtime_hook.cpp
===
--- /dev/null
+++ clang/test/CoverageMapping/unused_function_no_runtime_hook.cpp
@@ -0,0 +1,6 @@
+// RUN: %clang -target x86_64-unknown-fuchsia -fprofile-instr-generate 
-fcoverage-mapping -emit-llvm -S %s -o - | FileCheck %s
+
+// CHECK-NOT: @__llvm_profile_runtime
+static int f0() {
+  return 100;
+}


Index: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
===
--- llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
+++ llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
@@ -558,16 +558,20 @@
   TT = Triple(M.getTargetTriple());
 
   bool MadeChange = false;
-
-  // Emit the runtime hook even if no counters are present.
-  if (needsRuntimeHookUnconditionally(TT))
+  bool NeedsRuntimeHook = needsRuntimeHookUnconditionally(TT);
+  if (NeedsRuntimeHook)
 MadeChange = emitRuntimeHook();
 
   // Improve compile time by avoiding linear scans when there is no work.
   GlobalVariable *CoverageNamesVar =
   M.getNamedGlobal(getCoverageUnusedNamesVarName());
-  if (!containsProfilingIntrinsics(M) && !CoverageNamesVar)
-return MadeChange;
+  if (!containsProfilingIntrinsics(M)) {
+if (!CoverageNamesVar) {
+  return MadeChange;
+} else if (!NeedsRuntimeHook) {
+  return MadeChange;
+}
+  }
 
   // We did not know how many value sites there would be inside
   // the instrumented function. This is counting the number of instrumented
Index: clang/test/CoverageMapping/unused_function_no_runtime_hook.cpp
===
--- /dev/null
+++ clang/test/CoverageMapping/unused_function_no_runtime_hook.cpp
@@ -0,0 +1,6 @@
+// RUN: %clang -target x86_64-unknown-fuchsia -fprofile-instr-generate -fcoverage-mapping -emit-llvm -S %s -o - | FileCheck %s
+
+// CHECK-NOT: @__llvm_profile_runtime
+static int f0() {
+  return 100;
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118862: [clang][driver] add clang driver support for emitting macho files with two build version load commands

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

The following two tests started failing in our bots:

  Clang :: Driver/darwin-ld-platform-version-target-version.c
  Clang :: Driver/darwin-zippered-target-version.c

https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8822219707275734081/overview

The error message is as the following:

  Script:
  --
  : 'RUN: at line 1';   touch 
/b/s/w/ir/x/w/staging/llvm_build/tools/clang/test/Driver/Output/darwin-ld-platform-version-target-version.c.tmp.o
  : 'RUN: at line 3';   /b/s/w/ir/x/w/staging/llvm_build/bin/clang -target 
x86_64-apple-ios13.1-macabi -darwin-target-variant x86_64-apple-macos10.15 
-isysroot 
/b/s/w/ir/x/w/llvm-llvm-project/clang/test/Driver/Inputs/MacOSX10.15.versioned.sdk
 -mlinker-version=520 -### 
/b/s/w/ir/x/w/staging/llvm_build/tools/clang/test/Driver/Output/darwin-ld-platform-version-target-version.c.tmp.o
 2>&1| /b/s/w/ir/x/w/staging/llvm_build/bin/FileCheck 
/b/s/w/ir/x/w/llvm-llvm-project/clang/test/Driver/darwin-ld-platform-version-target-version.c
  : 'RUN: at line 5';   /b/s/w/ir/x/w/staging/llvm_build/bin/clang -target 
x86_64-apple-macos10.14.3 -darwin-target-variant x86_64-apple-ios13.1-macabi 
-isysroot 
/b/s/w/ir/x/w/llvm-llvm-project/clang/test/Driver/Inputs/MacOSX10.15.versioned.sdk
 -mlinker-version=520 -### 
/b/s/w/ir/x/w/staging/llvm_build/tools/clang/test/Driver/Output/darwin-ld-platform-version-target-version.c.tmp.o
 2>&1| /b/s/w/ir/x/w/staging/llvm_build/bin/FileCheck 
--check-prefix=CHECK-INV 
/b/s/w/ir/x/w/llvm-llvm-project/clang/test/Driver/darwin-ld-platform-version-target-version.c
  : 'RUN: at line 8';   /b/s/w/ir/x/w/staging/llvm_build/bin/clang -target 
arm64-apple-ios13.1-macabi -darwin-target-variant arm64-apple-macos10.15 
-isysroot 
/b/s/w/ir/x/w/llvm-llvm-project/clang/test/Driver/Inputs/MacOSX10.15.versioned.sdk
 -mlinker-version=520 -### 
/b/s/w/ir/x/w/staging/llvm_build/tools/clang/test/Driver/Output/darwin-ld-platform-version-target-version.c.tmp.o
 2>&1| /b/s/w/ir/x/w/staging/llvm_build/bin/FileCheck 
--check-prefix=ARM64_NEW 
/b/s/w/ir/x/w/llvm-llvm-project/clang/test/Driver/darwin-ld-platform-version-target-version.c
  : 'RUN: at line 10';   /b/s/w/ir/x/w/staging/llvm_build/bin/clang -target 
arm64-apple-macos10.15 -darwin-target-variant arm64-apple-ios13.1-macabi  
-isysroot 
/b/s/w/ir/x/w/llvm-llvm-project/clang/test/Driver/Inputs/MacOSX10.15.versioned.sdk
 -mlinker-version=520 -### 
/b/s/w/ir/x/w/staging/llvm_build/tools/clang/test/Driver/Output/darwin-ld-platform-version-target-version.c.tmp.o
 2>&1| /b/s/w/ir/x/w/staging/llvm_build/bin/FileCheck 
--check-prefix=ARM64_NEW-INV 
/b/s/w/ir/x/w/llvm-llvm-project/clang/test/Driver/darwin-ld-platform-version-target-version.c
  : 'RUN: at line 12';   /b/s/w/ir/x/w/staging/llvm_build/bin/clang -target 
arm64-apple-ios13.1-macabi -darwin-target-variant arm64-apple-macos10.15 
-isysroot 
/b/s/w/ir/x/w/llvm-llvm-project/clang/test/Driver/Inputs/MacOSX10.15.versioned.sdk
 -mlinker-version=400 -### 
/b/s/w/ir/x/w/staging/llvm_build/tools/clang/test/Driver/Output/darwin-ld-platform-version-target-version.c.tmp.o
 2>&1| /b/s/w/ir/x/w/staging/llvm_build/bin/FileCheck 
--check-prefix=ARM64_OLD 
/b/s/w/ir/x/w/llvm-llvm-project/clang/test/Driver/darwin-ld-platform-version-target-version.c
  : 'RUN: at line 14';   /b/s/w/ir/x/w/staging/llvm_build/bin/clang -target 
arm64-apple-macos10.15 -darwin-target-variant arm64-apple-ios13.1-macabi 
-isysroot 
/b/s/w/ir/x/w/llvm-llvm-project/clang/test/Driver/Inputs/MacOSX10.15.versioned.sdk
 -mlinker-version=400 -### 
/b/s/w/ir/x/w/staging/llvm_build/tools/clang/test/Driver/Output/darwin-ld-platform-version-target-version.c.tmp.o
 2>&1| /b/s/w/ir/x/w/staging/llvm_build/bin/FileCheck 
--check-prefix=ARM64_OLD-INV 
/b/s/w/ir/x/w/llvm-llvm-project/clang/test/Driver/darwin-ld-platform-version-target-version.c
  --
  Exit Code: 1
  
  Command Output (stderr):
  --
  
/b/s/w/ir/x/w/llvm-llvm-project/clang/test/Driver/darwin-ld-platform-version-target-version.c:29:15:
 error: ARM64_OLD: expected string not found in input
  // ARM64_OLD: "-maccatalyst_version_min" "14.0.0" "-macosx_version_min" 
"11.0.0"
^
  :1:1: note: scanning from here
  Fuchsia clang version 15.0.0 (https://llvm.googlesource.com/a/llvm-project 
cccef321096c20825fe8738045c1d91d3b9fd57d)
  ^
  :5:109: note: possible intended match here
   "/b/s/w/ir/x/w/staging/llvm_build/bin/ld64.lld" "-demangle" "-dynamic" 
"-arch" "arm64" "-platform_version" "mac catalyst" "14.0.0" "13.1" 
"-platform_version" "macos" "11.0.0" "10.15" "-syslibroot" 
"/b/s/w/ir/x/w/llvm-llvm-project/clang/test/Driver/Inputs/MacOSX10.15.versioned.sdk"
 "-o" "a.out" 
"/b/s/w/ir/x/w/staging/llvm_build/tools/clang/test/Driver/Output/darwin-ld-platform-version-target-version.c.tmp.o"
 "-lSystem"

  ^
  
  Input file: 
  Check file: 

[PATCH] D94355: [Passes] Add relative lookup table converter pass

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added inline comments.



Comment at: llvm/lib/Transforms/Utils/RelLookupTableConverter.cpp:132
+  // Place new instruction sequence after GEP.
+  Builder.SetInsertPoint(GEP);
+  Value *Index = GEP->getOperand(2);

jrtc27 wrote:
> This line causes the bug seen in bind. In that case, the GEP has been 
> hoisted, but the load has not. In general the GEP could be in a different 
> basic block, or even in the same basic block with an instruction that may not 
> return (intrinsic, real function call, well-defined language-level exception, 
> etc). You can insert the reltable.shift where the GEP is, and that probably 
> makes sense given it serves (part of) the same purpose, but you *must* insert 
> the actual reltable.intrinsic where the original load is, unless you've gone 
> to great lengths to prove it's safe not to (which seems best left to the 
> usual culprits like LICM).
> 
> IR test cases: https://godbolt.org/z/YMdaMrobE (bind is characterised by the 
> first of the two functions)
@dim and @jrtc27 thank you for reporting it.
I see what's going wrong, and I uploaded a patch that fixes the issue by 
ensuring that the call to load.relative.intrinsic is inserted before the load, 
but not gep.
Please see https://reviews.llvm.org/D115571. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355

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

[PATCH] D103938: Diagnose -Wunused-value based on CFG reachability

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

We also started seeing similar `libc++` test failures in our Fuchsia builds:
https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket/8835548361443044001/+/u/clang/test/stdout

  Failed Tests (89):
libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/assign.pass.cpp
libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/copy.pass.cpp
libc++ :: 
std/numerics/rand/rand.adapt/rand.adapt.ibits/ctor_result_type.pass.cpp
libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/ctor_sseq.pass.cpp
libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/default.pass.cpp
libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/discard.pass.cpp
libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/eval.pass.cpp
libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/io.pass.cpp
libc++ :: 
std/numerics/rand/rand.adapt/rand.adapt.ibits/seed_result_type.pass.cpp
libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/seed_sseq.pass.cpp
libc++ :: std/numerics/rand/rand.adapt/rand.adapt.ibits/values.pass.cpp
libc++ :: 
std/numerics/rand/rand.dis/rand.dist.bern/rand.dist.bern.bin/eval.pass.cpp
libc++ :: 
std/numerics/rand/rand.dis/rand.dist.bern/rand.dist.bern.bin/eval_param.pass.cpp
libc++ :: 
std/numerics/rand/rand.dis/rand.dist.samp/rand.dist.samp.pconst/eval.pass.cpp
libc++ :: 
std/numerics/rand/rand.dis/rand.dist.samp/rand.dist.samp.pconst/eval_param.pass.cpp
libc++ :: 
std/numerics/rand/rand.dis/rand.dist.samp/rand.dist.samp.plinear/eval.pass.cpp
libc++ :: 
std/numerics/rand/rand.dis/rand.dist.samp/rand.dist.samp.plinear/eval_param.pass.cpp
libc++ :: 
std/numerics/rand/rand.dis/rand.dist.uni/rand.dist.uni.int/eval.pass.cpp
libc++ :: 
std/numerics/rand/rand.dis/rand.dist.uni/rand.dist.uni.real/eval.pass.cpp
libc++ :: std/numerics/rand/rand.eng/rand.eng.lcong/assign.pass.cpp
libc++ :: std/numerics/rand/rand.eng/rand.eng.lcong/copy.pass.cpp
libc++ :: 
std/numerics/rand/rand.eng/rand.eng.lcong/ctor_result_type.pass.cpp
libc++ :: std/numerics/rand/rand.eng/rand.eng.lcong/default.pass.cpp
libc++ :: std/numerics/rand/rand.eng/rand.eng.lcong/result_type.pass.cpp
libc++ :: std/numerics/rand/rand.eng/rand.eng.lcong/values.pass.cpp
libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/assign.pass.cpp
libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/copy.pass.cpp
libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/ctor_result_type.pass.cpp
libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/ctor_sseq.pass.cpp
libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/default.pass.cpp
libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/discard.pass.cpp
libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/eval.pass.cpp
libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/io.pass.cpp
libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/result_type.pass.cpp
libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/seed_result_type.pass.cpp
libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/seed_sseq.pass.cpp
libc++ :: std/numerics/rand/rand.eng/rand.eng.mers/values.pass.cpp
libc++ :: std/numerics/rand/rand.predef/mt19937_64.pass.cpp
libc++ :: std/utilities/template.bitset/bitset.cons/char_ptr_ctor.pass.cpp
libc++ :: std/utilities/template.bitset/bitset.cons/default.pass.cpp
libc++ :: std/utilities/template.bitset/bitset.cons/string_ctor.pass.cpp
libc++ :: std/utilities/template.bitset/bitset.cons/ull_ctor.pass.cpp
libc++ :: std/utilities/template.bitset/bitset.hash/bitset.pass.cpp
libc++ :: std/utilities/template.bitset/bitset.hash/enabled_hash.pass.cpp
libc++ :: std/utilities/template.bitset/bitset.members/all.pass.cpp
libc++ :: std/utilities/template.bitset/bitset.members/any.pass.cpp
libc++ :: std/utilities/template.bitset/bitset.members/count.pass.cpp
libc++ :: std/utilities/template.bitset/bitset.members/flip_all.pass.cpp
libc++ :: 
std/utilities/template.bitset/bitset.members/flip_one.out_of_range.pass.cpp
libc++ :: std/utilities/template.bitset/bitset.members/flip_one.pass.cpp
libc++ :: std/utilities/template.bitset/bitset.members/index.pass.cpp
libc++ :: std/utilities/template.bitset/bitset.members/index_const.pass.cpp
libc++ :: std/utilities/template.bitset/bitset.members/left_shift.pass.cpp
libc++ :: 
std/utilities/template.bitset/bitset.members/left_shift_eq.pass.cpp
libc++ :: std/utilities/template.bitset/bitset.members/none.pass.cpp
libc++ :: std/utilities/template.bitset/bitset.members/not_all.pass.cpp
libc++ :: std/utilities/template.bitset/bitset.members/op_and_eq.pass.cpp
libc++ :: std/utilities/template.bitset/bitset.members/op_eq_eq.pass.cpp
libc++ :: std/utilities/template.bitset/bitset.members/op_or_eq.pass.cpp
libc++ :: std/utilities/template.bitset/bitset.members/op_xor_eq.pass.cpp
libc++ :: std/utilities/template.bitset/bitset.members/reset_all.pass.cpp
   

[PATCH] D102531: PR45881: Properly use CXXThisOverride for templated lambda

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

> I'll commit this by the end of the day if @rsmith wasn't able to take a look.

Sounds good, thank you very much!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102531

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

[PATCH] D102531: PR45881: Properly use CXXThisOverride for templated lambda

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

Any update on this review? Our builds are still broken.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102531

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

[PATCH] D108696: [Coroutines] [Frontend] Lookup in std namespace first

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

We started seeing the following test failures in our Fuchsia builds after this 
patch:

  Failed Tests (10):
libc++ :: 
libcxx/experimental/language.support/support.coroutines/dialect_support.pass.cpp
libc++ :: 
std/experimental/language.support/support.coroutines/coroutine.handle/coroutine.handle.prom/promise.pass.cpp
libc++ :: 
std/experimental/language.support/support.coroutines/end.to.end/await_result.pass.cpp
libc++ :: 
std/experimental/language.support/support.coroutines/end.to.end/bool_await_suspend.pass.cpp
libc++ :: 
std/experimental/language.support/support.coroutines/end.to.end/expected.pass.cpp
libc++ :: 
std/experimental/language.support/support.coroutines/end.to.end/fullexpr-dtor.pass.cpp
libc++ :: 
std/experimental/language.support/support.coroutines/end.to.end/generator.pass.cpp
libc++ :: 
std/experimental/language.support/support.coroutines/end.to.end/go.pass.cpp
libc++ :: 
std/experimental/language.support/support.coroutines/end.to.end/multishot_func.pass.cpp
libc++ :: 
std/experimental/language.support/support.coroutines/end.to.end/oneshot_func.pass.cpp

https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket/8837144494402864417/+/u/clang/test/stdout

Do you have any idea what might be going wrong?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108696

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

[PATCH] D102531: PR45881: Properly use CXXThisOverride for templated lambda

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

@aaron.ballman would it be possible for you to review this patch, so we can fix 
our broken builds?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102531

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

[PATCH] D102531: PR45881: Properly use CXXThisOverride for templated lambda

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

We recently enabled assertions in our Fuchsia builds, and our builds started to 
fail:

  FAILED: obj/src/lib/storage/vfs/cpp/libcpp.fuchsia_vfs.cc.o
  ../../../recipe_cleanup/clangLhcV7J/bin/clang++ -MD -MF 
obj/src/lib/storage/vfs/cpp/libcpp.fuchsia_vfs.cc.o.d 
-D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS 
-D_LIBCPP_ENABLE_THREAD_SAFETY_ANNOTATIONS=1 -…
  clang++: clang/lib/Sema/SemaExprCXX.cpp:1144: clang::QualType 
adjustCVQualifiersForCXXThisWithinLambda(ArrayRef, clang::QualType, clang::DeclContext *, clang::ASTC…
  clang++: error: clang frontend command failed with exit code 134 (use -v to 
see invocation)

https://luci-milo.appspot.com/ui/p/fuchsia/builders/ci/clang_toolchain.core.x64-debug/b8837288282633988801/overview

This patch seems to be fixing the issue that we are running.
Is there a way to speed up the process in this review, so our broken builds can 
be fixed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102531

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

[PATCH] D94355: [Passes] Add relative lookup table converter pass

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added inline comments.



Comment at: llvm/lib/Transforms/Utils/RelLookupTableConverter.cpp:73
+  return false;
+
+// If an operand in the lookup table is not dso_local,

pcc wrote:
> In the version of the patch that you committed, you have this check here:
> ```
> // If operand is mutable, do not generate a relative lookup table.
> auto *GlovalVarOp = dyn_cast(GVOp);
> if (!GlovalVarOp || !GlovalVarOp->isConstant())
>   return false;
> ```
> 1. Nit: Gloval -> Global
> 2. Why is it important whether the referenced global is mutable? The pointer 
> itself is constant.
1. That's a typo, and I will fix that.
2. This optimization does not do a detailed analysis, and it is being 
conservative.
 In this case, `GlobalVar` points to the switch lookup table and 
`GlobalVarOp` points to the elements in the lookup table like strings.
 To make sure that relative arithmetic works, it just checks whether both 
`GlobalVar` and `GlobalVarOp` pointers are constants.
 Did you see an issue on that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355

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

[PATCH] D102286: [HWASan] Build separate LAM runtime on x86_64.

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

> Looks like the same issue I fixed in https://reviews.llvm.org/rGd97bab651185. 
>  Let me know if you have the issue after that commit.

Thanks, that resolved the build issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102286

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

[PATCH] D102286: [HWASan] Build separate LAM runtime on x86_64.

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

We started seeing test failures in our Windows builds in Fuchsia after this 
patch.

  FAILED: 
compiler-rt/lib/hwasan/CMakeFiles/RTHwasanAliases.aarch64.dir/hwasan_interceptors.cpp.o
 
  C:\b\s\w\ir\x\w\staging\llvm_build\.\bin\clang++.exe 
--target=aarch64-unknown-linux-gnu --sysroot=C:/b/s/w/ir/x/w/cipd/linux  
-DHWASAN_WITH_INTERCEPTORS=1 -D_DEBUG -D__STDC_CONSTANT_MACROS 
-D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS 
-IC:/b/s/w/ir/x/w/llvm-project/compiler-rt/lib/hwasan/.. 
--target=aarch64-unknown-linux-gnu -fPIC -fvisibility-inlines-hidden 
-Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra 
-Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers 
-Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type 
-Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment 
-Wstring-conversion -Wmisleading-indentation -fdiagnostics-color 
-ffunction-sections -fdata-sections 
-ffile-prefix-map=C:/b/s/w/ir/x/w/staging/llvm_build/runtimes/runtimes-aarch64-unknown-linux-gnu-bins=../staging/llvm_build/runtimes/runtimes-aarch64-unknown-linux-gnu-bins
 -ffile-prefix-map=C:/b/s/w/ir/x/w/llvm-project/= -no-canonical-prefixes -Wall 
-std=c++14 -Wno-unused-parameter -O2 -g -DNDEBUG-fPIC -fno-builtin 
-fno-exceptions -fomit-frame-pointer -funwind-tables -fno-stack-protector 
-fno-sanitize=safe-stack -fvisibility=hidden -fno-lto -O3 -gline-tables-only 
-Wno-gnu -Wno-variadic-macros -Wno-c99-extensions -nostdinc++ -fno-rtti -fPIC 
-ffreestanding -DHWASAN_ALIASING_MODE -UNDEBUG -MD -MT 
compiler-rt/lib/hwasan/CMakeFiles/RTHwasanAliases.aarch64.dir/hwasan_interceptors.cpp.o
 -MF 
compiler-rt\lib\hwasan\CMakeFiles\RTHwasanAliases.aarch64.dir\hwasan_interceptors.cpp.o.d
 -o 
compiler-rt/lib/hwasan/CMakeFiles/RTHwasanAliases.aarch64.dir/hwasan_interceptors.cpp.o
 -c C:/b/s/w/ir/x/w/llvm-project/compiler-rt/lib/hwasan/hwasan_interceptors.cpp
  In file included from 
C:/b/s/w/ir/x/w/llvm-project/compiler-rt/lib/hwasan/hwasan_interceptors.cpp:18:
  C:/b/s/w/ir/x/w/llvm-project/compiler-rt/lib/hwasan/hwasan.h:41:6: error: 
Aliasing mode is only supported on x86_64

The error message to the full build:
https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket.appspot.com/8846968772132677360/+/u/clang/build/stdout

The CMake commands:
https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket.appspot.com/8846968772132677360/+/u/clang/configure/execution_details

@morehouse do you have any idea?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102286

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

[PATCH] D94355: [Passes] Add relative lookup table converter pass

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

> I would say if it is going to take longer than the end of the week to fix the 
> issue, it might be nice to have it reverted so that other people's builds 
> continue to work (I know a few people who build with full LTO in a two stage 
> configuration every week).

Definitely! If I cannot fix it in two days,  I'll revert the change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355

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

[PATCH] D94355: [Passes] Add relative lookup table converter pass

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

@nathanchance do you prefer me to revert the patch first as it might take me a 
while to investigate it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355

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

[PATCH] D94355: [Passes] Add relative lookup table converter pass

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

> This patch breaks a two stage build with LTO:

Thanks for the report @nathanchance, and I'm looking at it now.
Is this failure from one of the bots?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355

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

[PATCH] D92191: [clang-scan-deps] Add support for clang-cl

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

> Thanks! We found the issue. I think I'll revert the patch, and will reland a 
> fixed version.

Great, thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92191

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

[PATCH] D92191: [clang-scan-deps] Add support for clang-cl

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

> @gulfem We're taking a look now. Could you please post a link to the full 
> build log, including the cmake flags used?

This is the link to one of our failing builds:
https://ci.chromium.org/ui/p/fuchsia/builders/ci/clang-linux-arm64/b8849674814150864912/overview

Here, you can see the `cmake` flags:
https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket.appspot.com/8849674814150864912/+/u/clang/configure/execution_details


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92191

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

[PATCH] D92191: [clang-scan-deps] Add support for clang-cl

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

This seems to break our `Fucshia` builds.
If we cannot do a quick fix, we need to revert this patch.
Please see the error message below:

  FAIL: Clang :: ClangScanDeps/vfsoverlay.cpp (1863 of 27822)
   TEST 'Clang :: ClangScanDeps/vfsoverlay.cpp' FAILED 

  Script:
  --
  : 'RUN: at line 1';   rm -rf 
/opt/s/w/ir/x/w/staging/llvm_build/tools/clang/test/ClangScanDeps/Output/vfsoverlay.cpp.tmp.dir
  : 'RUN: at line 2';   rm -rf 
/opt/s/w/ir/x/w/staging/llvm_build/tools/clang/test/ClangScanDeps/Output/vfsoverlay.cpp.tmp.cdb
  : 'RUN: at line 3';   mkdir -p 
/opt/s/w/ir/x/w/staging/llvm_build/tools/clang/test/ClangScanDeps/Output/vfsoverlay.cpp.tmp.dir
  : 'RUN: at line 4';   cp 
/opt/s/w/ir/x/w/llvm-project/clang/test/ClangScanDeps/vfsoverlay.cpp 
/opt/s/w/ir/x/w/staging/llvm_build/tools/clang/test/ClangScanDeps/Output/vfsoverlay.cpp.tmp.dir/vfsoverlay_input.cpp
  : 'RUN: at line 5';   cp 
/opt/s/w/ir/x/w/llvm-project/clang/test/ClangScanDeps/vfsoverlay.cpp 
/opt/s/w/ir/x/w/staging/llvm_build/tools/clang/test/ClangScanDeps/Output/vfsoverlay.cpp.tmp.dir/vfsoverlay_input_clangcl.cpp
  : 'RUN: at line 6';   sed -e 
"s|DIR|/opt/s/w/ir/x/w/staging/llvm_build/tools/clang/test/ClangScanDeps/Output/vfsoverlay.cpp.tmp.dir|g"
 /opt/s/w/ir/x/w/llvm-project/clang/test/ClangScanDeps/Inputs/vfsoverlay.yaml > 
/opt/s/w/ir/x/w/staging/llvm_build/tools/clang/test/ClangScanDeps/Output/vfsoverlay.cpp.tmp.dir/vfsoverlay.yaml
  : 'RUN: at line 7';   mkdir 
/opt/s/w/ir/x/w/staging/llvm_build/tools/clang/test/ClangScanDeps/Output/vfsoverlay.cpp.tmp.dir/Inputs
  : 'RUN: at line 8';   cp 
/opt/s/w/ir/x/w/llvm-project/clang/test/ClangScanDeps/Inputs/header.h 
/opt/s/w/ir/x/w/staging/llvm_build/tools/clang/test/ClangScanDeps/Output/vfsoverlay.cpp.tmp.dir/Inputs/header.h
  : 'RUN: at line 9';   sed -e 
"s|DIR|/opt/s/w/ir/x/w/staging/llvm_build/tools/clang/test/ClangScanDeps/Output/vfsoverlay.cpp.tmp.dir|g"
 
/opt/s/w/ir/x/w/llvm-project/clang/test/ClangScanDeps/Inputs/vfsoverlay_cdb.json
 > 
/opt/s/w/ir/x/w/staging/llvm_build/tools/clang/test/ClangScanDeps/Output/vfsoverlay.cpp.tmp.cdb
  : 'RUN: at line 11';   clang-scan-deps -compilation-database 
/opt/s/w/ir/x/w/staging/llvm_build/tools/clang/test/ClangScanDeps/Output/vfsoverlay.cpp.tmp.cdb
 -j 1 |/opt/s/w/ir/x/w/staging/llvm_build/bin/FileCheck 
/opt/s/w/ir/x/w/llvm-project/clang/test/ClangScanDeps/vfsoverlay.cpp
  --
  Exit Code: 1
  
  Command Output (stderr):
  --
  /opt/s/w/ir/x/w/llvm-project/clang/test/ClangScanDeps/vfsoverlay.cpp:20:11: 
error: CHECK: expected string not found in input
  // CHECK: vfsoverlay_input_clangcl.o
^
  :3:113: note: scanning from here
   
/opt/s/w/ir/x/w/staging/llvm_build/tools/clang/test/ClangScanDeps/Output/vfsoverlay.cpp.tmp.dir/Inputs/header.h

  ^
  :5:98: note: possible intended match here
   
/opt/s/w/ir/x/w/staging/llvm_build/tools/clang/test/ClangScanDeps/Output/vfsoverlay.cpp.tmp.dir/vfsoverlay_input_clangcl.cpp
 \

   ^
  
  Input file: 
  Check file: 
/opt/s/w/ir/x/w/llvm-project/clang/test/ClangScanDeps/vfsoverlay.cpp
  
  -dump-input=help explains the following input dump.
  
  Input was:
  <<
  1: 
/opt/s/w/ir/x/w/staging/llvm_build/tools/clang/test/ClangScanDeps/Output/vfsoverlay.cpp.tmp.dir/vfsoverlay_input.o:
 \ 
  2:  
/opt/s/w/ir/x/w/staging/llvm_build/tools/clang/test/ClangScanDeps/Output/vfsoverlay.cpp.tmp.dir/vfsoverlay_input.cpp
 \ 
  3:  
/opt/s/w/ir/x/w/staging/llvm_build/tools/clang/test/ClangScanDeps/Output/vfsoverlay.cpp.tmp.dir/Inputs/header.h
 
  check:20'0
 X error: no match found
  4: 
pt/s/w/ir/x/w/staging/llvm_build/tools/clang/test/ClangScanDeps/Output/vfsoverlay.cpp.tmp.dir/vfsoverlay.yaml:
 \ 
  check:20'0 
~
  5:  
/opt/s/w/ir/x/w/staging/llvm_build/tools/clang/test/ClangScanDeps/Output/vfsoverlay.cpp.tmp.dir/vfsoverlay_input_clangcl.cpp
 \ 
  check:20'0 

  check:20'1
  ?   possible 
intended match
  6:  
/opt/s/w/ir/x/w/staging/llvm_build/tools/clang/test/ClangScanDeps/Output/vfsoverlay.cpp.tmp.dir/Inputs/header.h
 
  check:20'0 
~
  >>


Repository:

[PATCH] D94355: [Passes] Add relative lookup table converter pass

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

> FYI, I pushed the fix for the aarch64-coff issue now (D99572 
> , rGd5c5cf5ce8d921fc8c5e1b608c298a1ffa688d37 
> ) and 
> pushed another commit to remove the code for disabling the pass on aarch64 
> (rG57b259a852a6383880f5d0875d848420bb3c2945 
> ).

Thank you @mstorsjo!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355

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

[PATCH] D94355: [Passes] Add relative lookup table converter pass

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

> Would you be ok with reverting this change until I can sort that out, or can 
> we disable the pass for those targets until then?

I will disable the pass for those targets for now.
When the issue is resolved, I would like to enable it for those targets as well.




Comment at: llvm/test/Other/opt-O3-pipeline-enable-matrix.ll:322-323
 ; CHECK-NEXT:   Simplify the CFG
+; CHECK-NEXT: Relative Lookup Table Converter
+; CHECK-NEXT: FunctionPass Manager
 ; CHECK-NEXT:   Annotation Remarks

aeubanks wrote:
> aeubanks wrote:
> > rnk wrote:
> > > Putting a ModulePass in the middle of the CodeGen pass pipeline creates a 
> > > "pass barrier": now instead of applying every pass to each function in 
> > > turn, the old pass manager will stop, run this whole-module pass, and 
> > > then run subseqeunt passes in the next function pass manager on each 
> > > function in turn. This isn't ideal. @aeubanks, can you follow-up to make 
> > > sure this is addressed?
> > > 
> > > We had the same issues with the SymbolRewriter pass, which if you grep 
> > > for "Rewrite Symbols" you can see has the same issue. I remember writing 
> > > a patch to fix it, but I guess I never landed it.
> > I see "Rewrite Symbols" in the codegen pipeline and yeah it's splitting the 
> > function pass manager.
> > 
> > For this patch, can we just not add the pass to the legacy PM pipeline? 
> > It's deprecated and the new PM is already the default for the optimization 
> > pipeline.
> (https://reviews.llvm.org/D99707 for anybody interested)
> For this patch, can we just not add the pass to the legacy PM pipeline? It's 
> deprecated and the new PM is already the default for the optimization 
> pipeline.
@rnk  @aeubanks
If it causes issues, I'm ok to remove it from the legacy PM pipeline.
When I land this patch, I'll only add it to new PM. 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355

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

[PATCH] D94355: [Passes] Add relative lookup table converter pass

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added inline comments.



Comment at: llvm/utils/gn/secondary/llvm/lib/Transforms/Utils/BUILD.gn:64
 "PromoteMemoryToRegister.cpp",
+"RelLookupTableConverter.cpp"
 "SSAUpdater.cpp",

thakis wrote:
> leonardchan wrote:
> > Good that you added this, but I think Nico has a bot that automatically 
> > updates these BUILD.gn files so manually updating them may not be necessary.
> Please don't touch gn files unless you use them. Simple file additions in 
> cmake files are synced automatically http://github.com/llvmgnsyncbot
> 
> You forgot to add the trailing comma and now I had to fix it up manually 
> instead of doing nothing :P
Sorry about that Niko. I can fix it, so you don't need to do anything.
Leo actually pointed that out, but I thought that manually changing it won't do 
any harm.
Apparently it did though!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355

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

[PATCH] D94355: [Passes] Add relative lookup table converter pass

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG78a65cd945d0: [Passes] Add relative lookup table converter 
pass (authored by gulfem).

Changed prior to commit:
  https://reviews.llvm.org/D94355?vs=331139=332445#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355

Files:
  llvm/docs/Passes.rst
  llvm/include/llvm/Analysis/TargetTransformInfo.h
  llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
  llvm/include/llvm/CodeGen/BasicTTIImpl.h
  llvm/include/llvm/InitializePasses.h
  llvm/include/llvm/Transforms/Scalar.h
  llvm/include/llvm/Transforms/Utils/RelLookupTableConverter.h
  llvm/lib/Analysis/TargetTransformInfo.cpp
  llvm/lib/Passes/PassBuilder.cpp
  llvm/lib/Passes/PassRegistry.def
  llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/RelLookupTableConverter.cpp
  llvm/lib/Transforms/Utils/Utils.cpp
  llvm/test/CodeGen/AMDGPU/opt-pipeline.ll
  llvm/test/Other/new-pm-defaults.ll
  llvm/test/Other/new-pm-thinlto-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-samplepgo-defaults.ll
  llvm/test/Other/opt-O2-pipeline.ll
  llvm/test/Other/opt-O3-pipeline-enable-matrix.ll
  llvm/test/Other/opt-O3-pipeline.ll
  llvm/test/Other/opt-Os-pipeline.ll
  llvm/test/Other/pass-pipelines.ll
  llvm/test/Transforms/RelLookupTableConverter/X86/no_relative_lookup_table.ll
  llvm/test/Transforms/RelLookupTableConverter/X86/relative_lookup_table.ll
  llvm/utils/gn/secondary/llvm/lib/Transforms/Utils/BUILD.gn

Index: llvm/utils/gn/secondary/llvm/lib/Transforms/Utils/BUILD.gn
===
--- llvm/utils/gn/secondary/llvm/lib/Transforms/Utils/BUILD.gn
+++ llvm/utils/gn/secondary/llvm/lib/Transforms/Utils/BUILD.gn
@@ -61,6 +61,7 @@
 "NameAnonGlobals.cpp",
 "PredicateInfo.cpp",
 "PromoteMemoryToRegister.cpp",
+"RelLookupTableConverter.cpp"
 "SSAUpdater.cpp",
 "SSAUpdaterBulk.cpp",
 "SampleProfileLoaderBaseUtil.cpp",
Index: llvm/test/Transforms/RelLookupTableConverter/X86/relative_lookup_table.ll
===
--- /dev/null
+++ llvm/test/Transforms/RelLookupTableConverter/X86/relative_lookup_table.ll
@@ -0,0 +1,310 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -rel-lookup-table-converter -relocation-model=pic -S | FileCheck %s
+; RUN: opt < %s -passes=rel-lookup-table-converter -relocation-model=pic -S | FileCheck %s
+target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@.str = private unnamed_addr constant [5 x i8] c"zero\00", align 1
+@.str.1 = private unnamed_addr constant [4 x i8] c"one\00", align 1
+@.str.2 = private unnamed_addr constant [4 x i8] c"two\00", align 1
+@.str.3 = private unnamed_addr constant [8 x i8] c"default\00", align 1
+@.str.4 = private unnamed_addr constant [6 x i8] c"three\00", align 1
+@.str.5 = private unnamed_addr constant [5 x i8] c"str1\00", align 1
+@.str.6 = private unnamed_addr constant [5 x i8] c"str2\00", align 1
+@.str.7 = private unnamed_addr constant [12 x i8] c"singlevalue\00", align 1
+
+@a1 = external global i32, align 4
+@b1 = external global i32, align 4
+@c1 = external global i32, align 4
+@d1 = external global i32, align 4
+
+@a2 = internal global i32 0, align 4
+@b2 = internal global i32 0, align 4
+@c2 = internal global i32 0, align 4
+@d2 = internal global i32 0, align 4
+
+@hidden0 = external hidden global i32, align 8
+@hidden1 = external hidden global i32, align 8
+@hidden2 = external hidden global i32, align 8
+@hidden3 = external hidden global i32, align 8
+
+@switch.table.no_dso_local = private unnamed_addr constant [3 x i32*] [i32* @a1, i32* @b1, i32* @c1], align 8
+
+@switch.table.dso_local = private unnamed_addr constant [3 x i32*] [i32* @a2, i32* @b2, i32* @c2], align 8
+
+@switch.table.hidden = private unnamed_addr constant [3 x i32*] [i32* @hidden0, i32* @hidden1, i32* @hidden2], align 8
+
+@switch.table.string_table = private unnamed_addr constant [3 x i8*]
+ [
+  i8* getelementptr inbounds ([5 x i8], [5 x i8]* @.str, i64 0, i64 0),
+  i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str.1, i64 0, i64 0),
+  i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str.2, i64 0, i64 0)
+ ], align 8
+
+@switch.table.string_table_holes = private unnamed_addr constant [4 x i8*]
+   [
+i8* getelementptr inbounds ([5 x i8], [5 x i8]* @.str, i64 0, i64 0),
+

[PATCH] D94355: [Passes] Add relative lookup table converter pass

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added inline comments.



Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:390-391
+
+if (!TM.getTargetTriple().isArch64Bit())
+  return false;
+

lebedev.ri wrote:
> gulfem wrote:
> > lebedev.ri wrote:
> > > 1. But all tests are using `x86_64` triple?
> > > 2. This is somewhat backwards. if the target wants to disable this, it 
> > > will need to override this function with `return false;`.
> > 1. Although I used `x86_64 triple`, this optimization can be applied to 
> > other 64-bit architectures too, because it not target dependent except 
> > `isArch64Bit` and `getCodeModel` check.
> > 2. Is there a target that you have in mind that we need to disable this 
> > optimization? 
> > I thought that it makes sense to enable this optimization by default on all 
> > the targets that can support it.
> > In case targets want to disable it, they can override it as you said.
> > How can we improve the implementation?
> > If you have suggestions, I'm happy to incorporate that.
> > 
> I'm sorry, i do not understand.
> Why does `!TM.getTargetTriple().isArch64Bit()` check exist?
> To me it reads as "if we aren't compiling for AArch64, don't build rel lookup 
> tables".
> Am i misreading this?
`isArch64Bit` checks whether we have a 64-bit architecture, right?
I don't think it specifically checks for `AArch64`, and it can cover other 
64-bit architectures like `x86_64` as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355

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

[PATCH] D94355: [Passes] Add relative lookup table converter pass

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

@lebedev.ri do you have further comments?
If not, I would like to submit this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355

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

[PATCH] D94355: [Passes] Add relative lookup table converter pass

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem marked 2 inline comments as done.
gulfem added inline comments.



Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:390-391
+
+if (!TM.getTargetTriple().isArch64Bit())
+  return false;
+

lebedev.ri wrote:
> 1. But all tests are using `x86_64` triple?
> 2. This is somewhat backwards. if the target wants to disable this, it will 
> need to override this function with `return false;`.
1. Although I used `x86_64 triple`, this optimization can be applied to other 
64-bit architectures too, because it not target dependent except `isArch64Bit` 
and `getCodeModel` check.
2. Is there a target that you have in mind that we need to disable this 
optimization? 
I thought that it makes sense to enable this optimization by default on all the 
targets that can support it.
In case targets want to disable it, they can override it as you said.
How can we improve the implementation?
If you have suggestions, I'm happy to incorporate that.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355

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

[PATCH] D94355: [Passes] Add relative lookup table converter pass

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem marked 4 inline comments as done.
gulfem added inline comments.



Comment at: llvm/lib/Transforms/Utils/RelLookupTableConverter.cpp:32
+  // If lookup table has more than one user,
+  // do not generate a relative lookup table.
+  if (!GV.hasOneUse())

hans wrote:
> It would be better if the comment said why. I suppose the reason is we need 
> to be sure there aren't other uses of the table, because then it can't be 
> replaced.
> 
> But it would be cool if a future version of this patch could handle when 
> there are multiple loads from the table which can all be replaced -- for 
> example this could happen if a function which uses a lookup table gets 
> inlined into multiple places.
I actually ran into the exact case that you described during testing, where a 
function that uses a switch gets inlined into multiple call sites :)
This is only to simplify the analysis, and I now added a TODO section to 
explore that later.



Comment at: 
llvm/test/Transforms/RelLookupTableConverter/switch_relative_lookup_table.ll:39
+; ; Relative switch lookup table for strings
+define i8* @string_table(i32 %cond) {
+  ; FNOPIC-LABEL: @string_table(

leonardchan wrote:
> It looks like this test case isn't much different from `string_table` in 
> `relative_lookup_table.ll`? If so, then this file could be removed.
I renamed this test case to no_relative_lookup_table.ll that checks the cases 
where relative lookup table should not be generated like in non-pic mode, 
medium or large code models, and 32 bit architectures, etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355

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

[PATCH] D94355: [Passes] Add relative lookup table converter pass

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem updated this revision to Diff 331139.
gulfem added a comment.

1. Add no_relative_lookup_table.ll test case that checks the cases where 
relative lookup table should not be generated
2. Add comments about dso_local check and single use check


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355

Files:
  llvm/docs/Passes.rst
  llvm/include/llvm/Analysis/TargetTransformInfo.h
  llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
  llvm/include/llvm/CodeGen/BasicTTIImpl.h
  llvm/include/llvm/InitializePasses.h
  llvm/include/llvm/Transforms/Scalar.h
  llvm/include/llvm/Transforms/Utils/RelLookupTableConverter.h
  llvm/lib/Analysis/TargetTransformInfo.cpp
  llvm/lib/Passes/PassBuilder.cpp
  llvm/lib/Passes/PassRegistry.def
  llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/RelLookupTableConverter.cpp
  llvm/lib/Transforms/Utils/Utils.cpp
  llvm/test/CodeGen/AMDGPU/opt-pipeline.ll
  llvm/test/Other/new-pm-defaults.ll
  llvm/test/Other/new-pm-thinlto-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-samplepgo-defaults.ll
  llvm/test/Other/opt-O2-pipeline.ll
  llvm/test/Other/opt-O3-pipeline-enable-matrix.ll
  llvm/test/Other/opt-O3-pipeline.ll
  llvm/test/Other/opt-Os-pipeline.ll
  llvm/test/Other/pass-pipelines.ll
  llvm/test/Transforms/RelLookupTableConverter/X86/no_relative_lookup_table.ll
  llvm/test/Transforms/RelLookupTableConverter/X86/relative_lookup_table.ll
  llvm/utils/gn/secondary/llvm/lib/Transforms/Utils/BUILD.gn

Index: llvm/utils/gn/secondary/llvm/lib/Transforms/Utils/BUILD.gn
===
--- llvm/utils/gn/secondary/llvm/lib/Transforms/Utils/BUILD.gn
+++ llvm/utils/gn/secondary/llvm/lib/Transforms/Utils/BUILD.gn
@@ -61,6 +61,7 @@
 "NameAnonGlobals.cpp",
 "PredicateInfo.cpp",
 "PromoteMemoryToRegister.cpp",
+"RelLookupTableConverter.cpp"
 "SSAUpdater.cpp",
 "SSAUpdaterBulk.cpp",
 "SampleProfileLoaderBaseUtil.cpp",
Index: llvm/test/Transforms/RelLookupTableConverter/X86/relative_lookup_table.ll
===
--- /dev/null
+++ llvm/test/Transforms/RelLookupTableConverter/X86/relative_lookup_table.ll
@@ -0,0 +1,310 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -rel-lookup-table-converter -relocation-model=pic -S | FileCheck %s
+; RUN: opt < %s -passes=rel-lookup-table-converter -relocation-model=pic -S | FileCheck %s
+target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@.str = private unnamed_addr constant [5 x i8] c"zero\00", align 1
+@.str.1 = private unnamed_addr constant [4 x i8] c"one\00", align 1
+@.str.2 = private unnamed_addr constant [4 x i8] c"two\00", align 1
+@.str.3 = private unnamed_addr constant [8 x i8] c"default\00", align 1
+@.str.4 = private unnamed_addr constant [6 x i8] c"three\00", align 1
+@.str.5 = private unnamed_addr constant [5 x i8] c"str1\00", align 1
+@.str.6 = private unnamed_addr constant [5 x i8] c"str2\00", align 1
+@.str.7 = private unnamed_addr constant [12 x i8] c"singlevalue\00", align 1
+
+@a1 = external global i32, align 4
+@b1 = external global i32, align 4
+@c1 = external global i32, align 4
+@d1 = external global i32, align 4
+
+@a2 = internal global i32 0, align 4
+@b2 = internal global i32 0, align 4
+@c2 = internal global i32 0, align 4
+@d2 = internal global i32 0, align 4
+
+@hidden0 = external hidden global i32, align 8
+@hidden1 = external hidden global i32, align 8
+@hidden2 = external hidden global i32, align 8
+@hidden3 = external hidden global i32, align 8
+
+@switch.table.no_dso_local = private unnamed_addr constant [3 x i32*] [i32* @a1, i32* @b1, i32* @c1], align 8
+
+@switch.table.dso_local = private unnamed_addr constant [3 x i32*] [i32* @a2, i32* @b2, i32* @c2], align 8
+
+@switch.table.hidden = private unnamed_addr constant [3 x i32*] [i32* @hidden0, i32* @hidden1, i32* @hidden2], align 8
+
+@switch.table.string_table = private unnamed_addr constant [3 x i8*]
+ [
+  i8* getelementptr inbounds ([5 x i8], [5 x i8]* @.str, i64 0, i64 0),
+  i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str.1, i64 0, i64 0),
+  i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str.2, i64 0, i64 0)
+ ], align 8
+
+@switch.table.string_table_holes = private unnamed_addr constant [4 x i8*]
+   [
+i8* getelementptr inbounds ([5 x i8], [5 x i8]* @.str, i64 0, i64 0),
+   

[PATCH] D94355: [Passes] Add relative lookup table converter pass

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem marked 7 inline comments as done.
gulfem added inline comments.



Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:384
 
+  bool shouldBuildRelLookupTables() {
+const TargetMachine  = getTLI()->getTargetMachine();

leonardchan wrote:
> Sorry, I think you might have explained this offline, but what was the reason 
> for needing this in TTI? I would've though this information could be found in 
> the `Module` (PIC/no PIC, 64-bit or not, code model). If it turns out all of 
> this is available in `Module`, then we could greatly simplify some logic here 
> by just checking this at the start of the pass run.
> 
> If TTI is needed, then perhaps it may be better to just inline all these 
> checks in `convertToRelativeLookupTables` since this is the only place this 
> is called. I think we would only want to keep this here as a virtual method 
> if we plan to have multiple TTI-impls overriding this.
Code model or PIC/noPIC is only set in the module if the user explicitly 
specifies them.
TTI hook is necessary to access target machine information like the default 
code model.
TTI is basically the interface to communicate between IR transformations and 
codegen. 

I think the checks cannot be moved into the pass because it uses the 
TargetMachine which is only available/visible in the TTI implementation itself.
That's why I added a function into TTI to do target machine specific checks.
Similar approach is used during lookup table generation 
(`shouldBuildLookupTables`) to check codegen info.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355

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

[PATCH] D94355: [Passes] Add relative lookup table converter pass

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem updated this revision to Diff 330405.
gulfem added a comment.

1. Used IsConstantOffsetFromGlobal() function
2. Added this pass to the end of legacy pass manager pipeline
3. Moved all the tests under a new test directory


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355

Files:
  llvm/docs/Passes.rst
  llvm/include/llvm/Analysis/TargetTransformInfo.h
  llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
  llvm/include/llvm/CodeGen/BasicTTIImpl.h
  llvm/include/llvm/InitializePasses.h
  llvm/include/llvm/Transforms/Scalar.h
  llvm/include/llvm/Transforms/Utils/RelLookupTableConverter.h
  llvm/lib/Analysis/TargetTransformInfo.cpp
  llvm/lib/Passes/PassBuilder.cpp
  llvm/lib/Passes/PassRegistry.def
  llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/RelLookupTableConverter.cpp
  llvm/lib/Transforms/Utils/Utils.cpp
  llvm/test/CodeGen/AMDGPU/opt-pipeline.ll
  llvm/test/Other/new-pm-defaults.ll
  llvm/test/Other/new-pm-thinlto-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-samplepgo-defaults.ll
  llvm/test/Other/opt-O2-pipeline.ll
  llvm/test/Other/opt-O3-pipeline-enable-matrix.ll
  llvm/test/Other/opt-O3-pipeline.ll
  llvm/test/Other/opt-Os-pipeline.ll
  llvm/test/Other/pass-pipelines.ll
  llvm/test/Transforms/RelLookupTableConverter/relative_lookup_table.ll
  llvm/test/Transforms/RelLookupTableConverter/switch_relative_lookup_table.ll
  llvm/utils/gn/secondary/llvm/lib/Transforms/Utils/BUILD.gn

Index: llvm/utils/gn/secondary/llvm/lib/Transforms/Utils/BUILD.gn
===
--- llvm/utils/gn/secondary/llvm/lib/Transforms/Utils/BUILD.gn
+++ llvm/utils/gn/secondary/llvm/lib/Transforms/Utils/BUILD.gn
@@ -61,6 +61,7 @@
 "NameAnonGlobals.cpp",
 "PredicateInfo.cpp",
 "PromoteMemoryToRegister.cpp",
+"RelLookupTableConverter.cpp"
 "SSAUpdater.cpp",
 "SSAUpdaterBulk.cpp",
 "SampleProfileLoaderBaseUtil.cpp",
Index: llvm/test/Transforms/RelLookupTableConverter/switch_relative_lookup_table.ll
===
--- /dev/null
+++ llvm/test/Transforms/RelLookupTableConverter/switch_relative_lookup_table.ll
@@ -0,0 +1,73 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -rel-lookup-table-converter -S | FileCheck %s --check-prefix=FNOPIC
+; RUN: opt < %s -rel-lookup-table-converter -relocation-model=pic -S | FileCheck %s --check-prefix=FPIC
+
+; RUN: opt < %s -passes=rel-lookup-table-converter -S | FileCheck %s --check-prefix=FNOPIC
+; RUN: opt < %s -passes=rel-lookup-table-converter -relocation-model=pic -S | FileCheck %s --check-prefix=FPIC
+target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@.str = private unnamed_addr constant [5 x i8] c"zero\00", align 1
+@.str.1 = private unnamed_addr constant [4 x i8] c"one\00", align 1
+@.str.2 = private unnamed_addr constant [4 x i8] c"two\00", align 1
+@.str.3 = private unnamed_addr constant [8 x i8] c"default\00", align 1
+
+@switch.table.string_table = private unnamed_addr constant [3 x i8*]
+ [
+  i8* getelementptr inbounds ([5 x i8], [5 x i8]* @.str, i64 0, i64 0),
+  i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str.1, i64 0, i64 0),
+  i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str.2, i64 0, i64 0)
+ ], align 8
+
+; Switch lookup table
+; FNOPIC: @switch.table.string_table = private unnamed_addr constant [3 x i8*]
+; FNOPIC-SAME: [
+; FNOPIC-SAME: i8* getelementptr inbounds ([5 x i8], [5 x i8]* @.str, i64 0, i64 0),
+; FNOPIC-SAME: i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str.1, i64 0, i64 0),
+; FNOPIC-SAME: i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str.2, i64 0, i64 0)
+; FNOPIC-SAME: ], align 8
+
+; Relative switch lookup table
+; FPIC: @reltable.string_table = private unnamed_addr constant [3 x i32]
+; FPIC-SAME: [
+; FPIC-SAME: i32 trunc (i64 sub (i64 ptrtoint ([5 x i8]* @.str to i64), i64 ptrtoint ([3 x i32]* @reltable.string_table to i64)) to i32),
+; FPIC-SAME: i32 trunc (i64 sub (i64 ptrtoint ([4 x i8]* @.str.1 to i64), i64 ptrtoint ([3 x i32]* @reltable.string_table to i64)) to i32),
+; FPIC-SAME: i32 trunc (i64 sub (i64 ptrtoint ([4 x i8]* @.str.2 to i64), i64 ptrtoint ([3 x i32]* @reltable.string_table to i64)) to i32)
+; FPIC-SAME: ], align 4
+
+; ; Relative switch lookup table for strings
+define i8* @string_table(i32 %cond) {
+  ; FNOPIC-LABEL: @string_table(
+  ; FNOPIC-NEXT:  entry:
+  ; FNOPIC-NEXT:[[TMP0:%.*]] = icmp ult i32 

[PATCH] D94355: [Passes] Add relative lookup table converter pass

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem updated this revision to Diff 328354.
gulfem added a comment.

Use TTI hook for target machine checks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355

Files:
  clang/test/CodeGen/switch-to-lookup-table.c
  llvm/docs/Passes.rst
  llvm/include/llvm/Analysis/TargetTransformInfo.h
  llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
  llvm/include/llvm/CodeGen/BasicTTIImpl.h
  llvm/include/llvm/InitializePasses.h
  llvm/include/llvm/Transforms/Scalar.h
  llvm/include/llvm/Transforms/Utils/RelLookupTableConverter.h
  llvm/lib/Analysis/TargetTransformInfo.cpp
  llvm/lib/Passes/PassBuilder.cpp
  llvm/lib/Passes/PassRegistry.def
  llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/RelLookupTableConverter.cpp
  llvm/lib/Transforms/Utils/Utils.cpp
  llvm/test/CodeGen/AMDGPU/opt-pipeline.ll
  llvm/test/Other/new-pm-defaults.ll
  llvm/test/Other/new-pm-thinlto-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-samplepgo-defaults.ll
  llvm/test/Other/opt-O2-pipeline.ll
  llvm/test/Other/opt-O3-pipeline-enable-matrix.ll
  llvm/test/Other/opt-O3-pipeline.ll
  llvm/test/Other/opt-Os-pipeline.ll
  llvm/test/Transforms/SimplifyCFG/X86/relative_lookup_table.ll
  llvm/utils/gn/secondary/llvm/lib/Transforms/Utils/BUILD.gn

Index: llvm/utils/gn/secondary/llvm/lib/Transforms/Utils/BUILD.gn
===
--- llvm/utils/gn/secondary/llvm/lib/Transforms/Utils/BUILD.gn
+++ llvm/utils/gn/secondary/llvm/lib/Transforms/Utils/BUILD.gn
@@ -61,6 +61,7 @@
 "NameAnonGlobals.cpp",
 "PredicateInfo.cpp",
 "PromoteMemoryToRegister.cpp",
+"RelLookupTableConverter.cpp"
 "SSAUpdater.cpp",
 "SSAUpdaterBulk.cpp",
 "SampleProfileLoaderBaseUtil.cpp",
Index: llvm/test/Transforms/SimplifyCFG/X86/relative_lookup_table.ll
===
--- /dev/null
+++ llvm/test/Transforms/SimplifyCFG/X86/relative_lookup_table.ll
@@ -0,0 +1,310 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -rel-lookup-table-converter -relocation-model=pic -S | FileCheck %s
+; RUN: opt < %s -passes=rel-lookup-table-converter -relocation-model=pic -S | FileCheck %s
+target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@.str = private unnamed_addr constant [5 x i8] c"zero\00", align 1
+@.str.1 = private unnamed_addr constant [4 x i8] c"one\00", align 1
+@.str.2 = private unnamed_addr constant [4 x i8] c"two\00", align 1
+@.str.3 = private unnamed_addr constant [8 x i8] c"default\00", align 1
+@.str.4 = private unnamed_addr constant [6 x i8] c"three\00", align 1
+@.str.5 = private unnamed_addr constant [5 x i8] c"str1\00", align 1
+@.str.6 = private unnamed_addr constant [5 x i8] c"str2\00", align 1
+@.str.7 = private unnamed_addr constant [12 x i8] c"singlevalue\00", align 1
+
+@a1 = external global i32, align 4
+@b1 = external global i32, align 4
+@c1 = external global i32, align 4
+@d1 = external global i32, align 4
+
+@a2 = internal global i32 0, align 4
+@b2 = internal global i32 0, align 4
+@c2 = internal global i32 0, align 4
+@d2 = internal global i32 0, align 4
+
+@hidden0 = external hidden global i32, align 8
+@hidden1 = external hidden global i32, align 8
+@hidden2 = external hidden global i32, align 8
+@hidden3 = external hidden global i32, align 8
+
+@switch.table.no_dso_local = private unnamed_addr constant [3 x i32*] [i32* @a1, i32* @b1, i32* @c1], align 8
+
+@switch.table.dso_local = private unnamed_addr constant [3 x i32*] [i32* @a2, i32* @b2, i32* @c2], align 8
+
+@switch.table.hidden = private unnamed_addr constant [3 x i32*] [i32* @hidden0, i32* @hidden1, i32* @hidden2], align 8
+
+@switch.table.string_table = private unnamed_addr constant [3 x i8*]
+ [
+  i8* getelementptr inbounds ([5 x i8], [5 x i8]* @.str, i64 0, i64 0),
+  i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str.1, i64 0, i64 0),
+  i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str.2, i64 0, i64 0)
+ ], align 8
+
+@switch.table.string_table_holes = private unnamed_addr constant [4 x i8*]
+   [
+i8* getelementptr inbounds ([5 x i8], [5 x i8]* @.str, i64 0, i64 0),
+i8* getelementptr inbounds ([8 x i8], [8 x i8]* @.str.3, i64 0, i64 0),
+i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str.2, i64 0, i64 0),
+i8* getelementptr 

[PATCH] D90275: [clang][IR] Add support for leaf attribute

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem closed this revision.
gulfem added a comment.

Since this patch has been merged and the issue in the test has been resolved, 
I'm closing this review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90275

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

[PATCH] D94355: [Passes] Add relative lookup table converter pass

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added inline comments.



Comment at: llvm/lib/Transforms/Utils/RelLookupTableGenerator.cpp:76-77
+
+  /// If lookup table has more than one user,
+  /// do not generate a relative lookup table.
+  if (!GlobalVar.hasOneUser())

leonardchan wrote:
> What's the reason for why the number of users matters?
We generate one lookup table per switch statement whenever possible.
I think there is only one use of that lookup table which is the`GetElementPtr` 
instruction.
That is why I checked for one use.
Do you see any issue with that?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355

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

[PATCH] D94355: [Passes] Add relative lookup table converter pass

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

In D94355#2572553 , @leonardchan wrote:

> It looks like you have everything setup for running on the new PM, but it 
> doesn't look like the pass is added anywhere in the new PM pipeline.

Thank you very much for pointing that out @leonardchan ! 
I added this pass into both pass managers now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355

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

[PATCH] D94355: [Passes] Add relative lookup table converter pass

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

> Thanks for pushing this forward! I think this will be a nice transformation 
> once all the details are worked out.

Thank you very much for all of your wonderful constructive feedback!
I learned much more about LLVM and IR internals. 
Appreciate all your help!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355

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

[PATCH] D94355: [Passes] Add relative lookup table converter pass

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem marked 17 inline comments as done.
gulfem added inline comments.



Comment at: clang/test/CodeGen/switch-to-lookup-table.c:2
+// Check switch to lookup optimization in fPIC and fno-PIC mode
+// RUN: %clang_cc1 %s -triple=aarch64-unknown-fuchsia -O2 -fno-PIC -S 
-emit-llvm -o - | FileCheck %s --check-prefix=CHECK --check-prefix=FNOPIC
+// RUN: %clang_cc1 %s -triple=aarch64-unknown-fuchsia -O2 -fPIC -S -emit-llvm 
-o - | FileCheck %s --check-prefix=CHECK --check-prefix=FPIC

hans wrote:
> gulfem wrote:
> > hans wrote:
> > > Clang codegen tests are not normally used to test LLVM optimizations. I 
> > > think the tests for this should all live in LLVM, not Clang.
> > > 
> > > (Also aarch64 is not guaranteed to be included as a target in the LLVM 
> > > build, so this test would not necessarily run.)
> > I'm not able to use -fPIC and -fno-PIC options in the `opt` tool.
> > I am setting the `PIC Level` flag to enable -fPIC in `opt.
> > I thought that testing -fPIC and -fno-PIC in the same file seems easier and 
> > more readable  in CodeGen tests.
> > Please let me know if you have a good suggestion how to do that with `opt`.
> > 
> > I changed the target to `x86_64-linux` in this test.
> Buildbots may not have x86_64 as a registered target, so this test will break 
> some buildbots.
> 
> I think the opt flags -relocation-model=pic and -relocation-model=static will 
> do the right thing (see for example 
> llvm/test/Transforms/SimplifyCFG/ARM/switch-to-lookup-table.ll
I added `x86-registered-target` to ensure that it only runs on buildbots that 
have `x86_64` as a registered target



Comment at: llvm/lib/Transforms/Utils/RelLookupTableGenerator.cpp:39
+
+  // If not in PIC mode, do not generate a relative lookup table.
+  if (M.getPICLevel() == PICLevel::NotPIC)

hans wrote:
> Again, this needs the "why".
> And perhaps it would make sense to put this check first.
I added the reason, please let me know if that's not clear enough.



Comment at: llvm/lib/Transforms/Utils/RelLookupTableGenerator.cpp:169
+
+  for (Module::global_iterator GVI = M.global_begin(), E = M.global_end();
+   GVI != E;) {

hans wrote:
> This would be simpler as
> 
> ```
> for (GlobalVariable *GlobalVar : M.globals()) {
> ```
Please keep that in mind that I'm deleting a global variable while I'm 
iterating over global variables.
I don't want to have invalidated iterator, and this is why I did not use a 
simple for loop.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355

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

[PATCH] D94355: [Passes] Add relative lookup table generator pass

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem updated this revision to Diff 326265.
gulfem added a comment.
Herald added subscribers: wenlei, nikic, kerbowa, steven_wu, nhaehnle, jvesely.

- Rename the pass to RelLookupTableConverter to be consistent
- Addressed reviewers' feedback
- Added tests for user-defined lookup tables and hidden visibility


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355

Files:
  clang/test/CodeGen/switch-to-lookup-table.c
  llvm/docs/Passes.rst
  llvm/include/llvm/InitializePasses.h
  llvm/include/llvm/Transforms/Scalar.h
  llvm/include/llvm/Transforms/Utils/RelLookupTableConverter.h
  llvm/lib/Passes/PassBuilder.cpp
  llvm/lib/Passes/PassRegistry.def
  llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/RelLookupTableConverter.cpp
  llvm/lib/Transforms/Utils/Utils.cpp
  llvm/test/CodeGen/AMDGPU/opt-pipeline.ll
  llvm/test/Other/new-pm-defaults.ll
  llvm/test/Other/new-pm-thinlto-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll
  llvm/test/Other/new-pm-thinlto-postlink-samplepgo-defaults.ll
  llvm/test/Other/opt-O2-pipeline.ll
  llvm/test/Other/opt-O3-pipeline-enable-matrix.ll
  llvm/test/Other/opt-O3-pipeline.ll
  llvm/test/Other/opt-Os-pipeline.ll
  llvm/test/Transforms/SimplifyCFG/X86/relative_lookup_table.ll
  llvm/utils/gn/secondary/llvm/lib/Transforms/Utils/BUILD.gn

Index: llvm/utils/gn/secondary/llvm/lib/Transforms/Utils/BUILD.gn
===
--- llvm/utils/gn/secondary/llvm/lib/Transforms/Utils/BUILD.gn
+++ llvm/utils/gn/secondary/llvm/lib/Transforms/Utils/BUILD.gn
@@ -60,6 +60,7 @@
 "NameAnonGlobals.cpp",
 "PredicateInfo.cpp",
 "PromoteMemoryToRegister.cpp",
+"RelLookupTableConverter.cpp"
 "SSAUpdater.cpp",
 "SSAUpdaterBulk.cpp",
 "SampleProfileLoaderBaseUtil.cpp",
Index: llvm/test/Transforms/SimplifyCFG/X86/relative_lookup_table.ll
===
--- /dev/null
+++ llvm/test/Transforms/SimplifyCFG/X86/relative_lookup_table.ll
@@ -0,0 +1,310 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -rel-lookup-table-converter -S | FileCheck %s
+; RUN: opt < %s -passes=rel-lookup-table-converter -S | FileCheck %s
+target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@.str = private unnamed_addr constant [5 x i8] c"zero\00", align 1
+@.str.1 = private unnamed_addr constant [4 x i8] c"one\00", align 1
+@.str.2 = private unnamed_addr constant [4 x i8] c"two\00", align 1
+@.str.3 = private unnamed_addr constant [8 x i8] c"default\00", align 1
+@.str.4 = private unnamed_addr constant [6 x i8] c"three\00", align 1
+@.str.5 = private unnamed_addr constant [5 x i8] c"str1\00", align 1
+@.str.6 = private unnamed_addr constant [5 x i8] c"str2\00", align 1
+@.str.7 = private unnamed_addr constant [12 x i8] c"singlevalue\00", align 1
+
+@a1 = external global i32, align 4
+@b1 = external global i32, align 4
+@c1 = external global i32, align 4
+@d1 = external global i32, align 4
+
+@a2 = internal global i32 0, align 4
+@b2 = internal global i32 0, align 4
+@c2 = internal global i32 0, align 4
+@d2 = internal global i32 0, align 4
+
+@hidden0 = external hidden global i32, align 8
+@hidden1 = external hidden global i32, align 8
+@hidden2 = external hidden global i32, align 8
+@hidden3 = external hidden global i32, align 8
+
+@switch.table.no_dso_local = private unnamed_addr constant [3 x i32*] [i32* @a1, i32* @b1, i32* @c1], align 8
+
+@switch.table.dso_local = private unnamed_addr constant [3 x i32*] [i32* @a2, i32* @b2, i32* @c2], align 8
+
+@switch.table.hidden = private unnamed_addr constant [3 x i32*] [i32* @hidden0, i32* @hidden1, i32* @hidden2], align 8
+
+@switch.table.string_table = private unnamed_addr constant [3 x i8*]
+ [
+  i8* getelementptr inbounds ([5 x i8], [5 x i8]* @.str, i64 0, i64 0),
+  i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str.1, i64 0, i64 0),
+  i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str.2, i64 0, i64 0)
+ ], align 8
+
+@switch.table.string_table_holes = private unnamed_addr constant [4 x i8*]
+   [
+i8* getelementptr inbounds ([5 x i8], [5 x i8]* @.str, i64 0, i64 0),
+i8* getelementptr inbounds ([8 x i8], [8 x i8]* @.str.3, i64 0, i64 0),
+i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str.2, i64 0, i64 0),
+i8* getelementptr inbounds ([6 x i8], [6 x i8]* 

[PATCH] D94355: [SimplifyCFG] Add relative switch lookup tables

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

@lebedev.ri based on your feedback, I added it as a separate pass and added 
support for user-defined lookup tables.
Please let me know if you have any comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355

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

[PATCH] D94355: [SimplifyCFG] Add relative switch lookup tables

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem updated this revision to Diff 324144.
gulfem marked an inline comment as not done.
gulfem added a comment.
Herald added a subscriber: mgorny.

Implement it as a separate pass and apply it to user-defined lookup tables as 
well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355

Files:
  clang/test/CodeGen/switch-to-lookup-table.c
  llvm/docs/Passes.rst
  llvm/include/llvm/InitializePasses.h
  llvm/include/llvm/Transforms/Scalar.h
  llvm/include/llvm/Transforms/Utils/RelLookupTableGenerator.h
  llvm/lib/Passes/PassBuilder.cpp
  llvm/lib/Passes/PassRegistry.def
  llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
  llvm/lib/Transforms/Utils/CMakeLists.txt
  llvm/lib/Transforms/Utils/RelLookupTableGenerator.cpp
  llvm/lib/Transforms/Utils/Utils.cpp
  llvm/test/Other/pass-pipelines.ll
  llvm/test/Transforms/SimplifyCFG/X86/relative_lookup_table.ll
  llvm/utils/gn/secondary/llvm/lib/Transforms/Utils/BUILD.gn

Index: llvm/utils/gn/secondary/llvm/lib/Transforms/Utils/BUILD.gn
===
--- llvm/utils/gn/secondary/llvm/lib/Transforms/Utils/BUILD.gn
+++ llvm/utils/gn/secondary/llvm/lib/Transforms/Utils/BUILD.gn
@@ -60,6 +60,7 @@
 "NameAnonGlobals.cpp",
 "PredicateInfo.cpp",
 "PromoteMemoryToRegister.cpp",
+"RelLookupTableGenerator.cpp"
 "SSAUpdater.cpp",
 "SSAUpdaterBulk.cpp",
 "SampleProfileLoaderBaseUtil.cpp",
Index: llvm/test/Transforms/SimplifyCFG/X86/relative_lookup_table.ll
===
--- /dev/null
+++ llvm/test/Transforms/SimplifyCFG/X86/relative_lookup_table.ll
@@ -0,0 +1,187 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -rel-lookup-table-generator -S -mtriple=x86_64-unknown-linux-gnu | FileCheck %s
+; RUN: opt < %s -passes=rel-lookup-table-generator -S -mtriple=x86_64-unknown-linux-gnu | FileCheck %s
+target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@.str = private unnamed_addr constant [5 x i8] c"zero\00", align 1
+@.str.1 = private unnamed_addr constant [4 x i8] c"one\00", align 1
+@.str.2 = private unnamed_addr constant [4 x i8] c"two\00", align 1
+@.str.3 = private unnamed_addr constant [8 x i8] c"default\00", align 1
+@.str.4 = private unnamed_addr constant [6 x i8] c"three\00", align 1
+@.str.5 = private unnamed_addr constant [5 x i8] c"str1\00", align 1
+@.str.6 = private unnamed_addr constant [5 x i8] c"str2\00", align 1
+@.str.7 = private unnamed_addr constant [12 x i8] c"singlevalue\00", align 1
+
+@switch.table.string_table = private unnamed_addr constant [3 x i8*]
+ [
+  i8* getelementptr inbounds ([5 x i8], [5 x i8]* @.str, i64 0, i64 0),
+  i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str.1, i64 0, i64 0),
+  i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str.2, i64 0, i64 0)
+ ], align 8
+
+@switch.table.string_table_holes = private unnamed_addr constant [4 x i8*]
+  [
+i8* getelementptr inbounds ([5 x i8], [5 x i8]* @.str, i64 0, i64 0),
+i8* getelementptr inbounds ([8 x i8], [8 x i8]* @.str.3, i64 0, i64 0),
+i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str.2, i64 0, i64 0),
+i8* getelementptr inbounds ([6 x i8], [6 x i8]* @.str.4, i64 0, i64 0)
+  ], align 8
+
+@switch.table.no_dso_local = private unnamed_addr constant [3 x i32*] [i32* @a, i32* @b, i32* @c], align 8
+
+@switch.table.single_value = private unnamed_addr constant [3 x i8*]
+  [
+i8* getelementptr inbounds ([5 x i8], [5 x i8]* @.str, i64 0, i64 0),
+i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str.1, i64 0, i64 0),
+i8* getelementptr inbounds ([4 x i8], [4 x i8]* @.str.2, i64 0, i64 0)
+  ], align 8
+
+; Integer pointer table lookup
+; CHECK: @switch.table.no_dso_local = private unnamed_addr constant [3 x i32*] [i32* @a, i32* @b, i32* @c], align
+
+; Relative string table lookup
+; CHECK: @reltable.string_table = private unnamed_addr constant [3 x i32]
+; CHECK-SAME: [
+; CHECK-SAME: i32 trunc (i64 sub (i64 ptrtoint ([5 x i8]* @.str to i64), i64 ptrtoint ([3 x i32]* @reltable.string_table to i64)) to i32),
+; CHECK-SAME: i32 trunc (i64 sub (i64 ptrtoint ([4 x i8]* @.str.1 to i64), i64 ptrtoint ([3 x i32]* @reltable.string_table to i64)) to i32),
+; CHECK-SAME: i32 trunc (i64 

[PATCH] D94355: [SimplifyCFG] Add relative switch lookup tables

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

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

In D94355#2530225 , @lebedev.ri wrote:

> Can you please add an explanation to the patch's description as to why
> we don't want to instead convert non-relative/relative LUT's elsewhere,
> please.

@mcgrathr gave some explanation to that:

>> Many backends generate PIC-friendly jump tables. This is about generating IR 
>> initializers that translate to the same kind of backend assembly expressions 
>> as those backends use for their jump tables.

I also want to add to that:
This task specifically tries make switch-to-lookup tables PIC-friendly, but it 
does not necessarily try to make all the jump tables PIC-friendly.
There is also another work going on to generate PIC-friendly vtables.
Therefore, converting non-relative lookup tables to relative tables elsewhere 
can be explored as a separate task.




Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:5512-5525
+  // If not in x86 or aarch64 mode, do not generate a relative lookup table.
+  Triple TargetTriple(M.getTargetTriple());
+  if (!(TargetTriple.getArch() == Triple::x86_64 ||
+TargetTriple.getArch() == Triple::aarch64))
+return false;
+
+  // If not tiny or small code model, do not generate a relative lookup table.

lebedev.ri wrote:
> This should be some TLI/TTI hook.
> This should be some TLI/TTI hook.
Could you please elaborate on that?
Are you talking about getting the PIC level?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355

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

[PATCH] D94355: [SimplifyCFG] Add relative switch lookup tables

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem marked 9 inline comments as done.
gulfem added inline comments.



Comment at: clang/test/CodeGen/switch-to-lookup-table.c:2
+// Check switch to lookup optimization in fPIC and fno-PIC mode
+// RUN: %clang_cc1 %s -triple=aarch64-unknown-fuchsia -O2 -fno-PIC -S 
-emit-llvm -o - | FileCheck %s --check-prefix=CHECK --check-prefix=FNOPIC
+// RUN: %clang_cc1 %s -triple=aarch64-unknown-fuchsia -O2 -fPIC -S -emit-llvm 
-o - | FileCheck %s --check-prefix=CHECK --check-prefix=FPIC

hans wrote:
> Clang codegen tests are not normally used to test LLVM optimizations. I think 
> the tests for this should all live in LLVM, not Clang.
> 
> (Also aarch64 is not guaranteed to be included as a target in the LLVM build, 
> so this test would not necessarily run.)
I'm not able to use -fPIC and -fno-PIC options in the `opt` tool.
I am setting the `PIC Level` flag to enable -fPIC in `opt.
I thought that testing -fPIC and -fno-PIC in the same file seems easier and 
more readable  in CodeGen tests.
Please let me know if you have a good suggestion how to do that with `opt`.

I changed the target to `x86_64-linux` in this test.



Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:5709
+  return llvm::ConstantExpr::getTrunc(RelOffset,
+  Type::getInt32Ty(M.getContext()));
 }

hans wrote:
> I do worry about hard-coding this to 32 bits. As someone pointed out, it 
> would not necessary hold in all code models for x86.
> 
> Similarly to the PIC check in ShouldBuildRelLookupTable(), is there some 
> check we could do to make sure 32 bits is appropriate?
I added `x86_64` and `aarch64` target check and tiny or small code mode check 
to ensure 32 offsets.
Please let me know if you have any other concerns about that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355

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

[PATCH] D94355: [SimplifyCFG] Add relative switch lookup tables

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem updated this revision to Diff 320025.
gulfem added a comment.

1. Simplified test cases and increased readibility of the tables
2. Added x86_64 and aarch64 check and tiny or small code modes check to ensure 
32 offsets
3. Modified single value test case


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355

Files:
  clang/test/CodeGen/switch-to-lookup-table.c
  llvm/lib/Transforms/Utils/SimplifyCFG.cpp
  llvm/test/Transforms/SimplifyCFG/X86/switch_to_relative_lookup_table.ll

Index: llvm/test/Transforms/SimplifyCFG/X86/switch_to_relative_lookup_table.ll
===
--- /dev/null
+++ llvm/test/Transforms/SimplifyCFG/X86/switch_to_relative_lookup_table.ll
@@ -0,0 +1,211 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -simplifycfg -switch-to-lookup=true -keep-loops=false -S -mtriple=x86_64-unknown-linux-gnu | FileCheck %s
+; RUN: opt < %s -passes='simplify-cfg' -S -mtriple=x86_64-unknown-linux-gnu | FileCheck %s
+target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@.str = private unnamed_addr constant [5 x i8] c"zero\00", align 1
+@.str.1 = private unnamed_addr constant [4 x i8] c"one\00", align 1
+@.str.2 = private unnamed_addr constant [4 x i8] c"two\00", align 1
+@.str.3 = private unnamed_addr constant [8 x i8] c"default\00", align 1
+@.str.4 = private unnamed_addr constant [6 x i8] c"three\00", align 1
+@.str.5 = private unnamed_addr constant [5 x i8] c"str1\00", align 1
+@.str.6 = private unnamed_addr constant [5 x i8] c"str2\00", align 1
+@.str.7 = private unnamed_addr constant [12 x i8] c"singlevalue\00", align 1
+
+; Relative string table lookup
+; CHECK: @switch.reltable.string_table = private unnamed_addr constant [3 x i32]
+; CHECK-SAME: [
+; CHECK-SAME: i32 trunc (i64 sub (i64 ptrtoint ([5 x i8]* @.str to i64), i64 ptrtoint ([3 x i32]* @switch.reltable.string_table to i64)) to i32),
+; CHECK-SAME: i32 trunc (i64 sub (i64 ptrtoint ([4 x i8]* @.str.1 to i64), i64 ptrtoint ([3 x i32]* @switch.reltable.string_table to i64)) to i32),
+; CHECK-SAME: i32 trunc (i64 sub (i64 ptrtoint ([4 x i8]* @.str.2 to i64), i64 ptrtoint ([3 x i32]* @switch.reltable.string_table to i64)) to i32)
+; CHECK-SAME: ], align 4
+
+; Relative string table lookup that holes are filled with relative offset to default values
+; CHECK: @switch.reltable.string_table_holes = private unnamed_addr constant [4 x i32]
+; CHECK-SAME: [
+; CHECK-SAME: i32 trunc (i64 sub (i64 ptrtoint ([5 x i8]* @.str to i64), i64 ptrtoint ([4 x i32]* @switch.reltable.string_table_holes to i64)) to i32),
+; CHECK-SAME: i32 trunc (i64 sub (i64 ptrtoint ([8 x i8]* @.str.3 to i64), i64 ptrtoint ([4 x i32]* @switch.reltable.string_table_holes to i64)) to i32),
+; CHECK-SAME: i32 trunc (i64 sub (i64 ptrtoint ([4 x i8]* @.str.2 to i64), i64 ptrtoint ([4 x i32]* @switch.reltable.string_table_holes to i64)) to i32),
+; CHECK-SAME: i32 trunc (i64 sub (i64 ptrtoint ([6 x i8]* @.str.4 to i64), i64 ptrtoint ([4 x i32]* @switch.reltable.string_table_holes to i64)) to i32)
+; CHECK-SAME: ], align 4
+
+; Integer pointer table lookup
+; CHECK: @switch.table.no_dso_local = private unnamed_addr constant [7 x i32*] [i32* @a, i32* @b, i32* @c, i32* @d, i32* @e, i32* @f, i32* @g], align 8
+
+; Single value check
+; CHECK: @switch.reltable.single_value = private unnamed_addr constant [3 x i32]
+; CHECK-SAME: [
+; CHECK-SAME: i32 trunc (i64 sub (i64 ptrtoint ([5 x i8]* @.str to i64), i64 ptrtoint ([3 x i32]* @switch.reltable.single_value to i64)) to i32),
+; CHECK-SAME: i32 trunc (i64 sub (i64 ptrtoint ([4 x i8]* @.str.1 to i64), i64 ptrtoint ([3 x i32]* @switch.reltable.single_value to i64)) to i32),
+; CHECK-SAME: i32 trunc (i64 sub (i64 ptrtoint ([4 x i8]* @.str.2 to i64), i64 ptrtoint ([3 x i32]* @switch.reltable.single_value to i64)) to i32)
+; CHECK-SAME: ], align 4
+
+; Switch used to return a string.
+; Relative lookup table should be generated.
+define i8* @string_table(i32 %cond) {
+  ; CHECK-LABEL: @string_table(
+  ; CHECK-NEXT:  entry:
+  ; CHECK-NEXT:[[TMP0:%.*]] = icmp ult i32 [[COND:%.*]], 3
+  ; CHECK-NEXT:br i1 [[TMP0]], label [[SWITCH_LOOKUP:%.*]], label [[RETURN:%.*]]
+  ; CHECK:   switch.lookup:
+  ; CHECK-NEXT:[[SWITCH_RELTABLE_SHIFT:%.*]] = lshr i32 %cond, 2
+  ; CHECK-NEXT:[[SWITCH_RELTABLE_INTRINSIC:%.*]] = call i8* @llvm.load.relative.i32(i8* bitcast ([3 x i32]* @switch.reltable.string_table to i8*), i32 [[SWITCH_RELTABLE_SHIFT]])
+  ; CHECK-NEXT:ret i8* [[SWITCH_RELTABLE_INTRINSIC]]
+  ; CHECK:   return:
+  ; CHECK-NEXT:ret i8* getelementptr inbounds ([8 x i8], [8 x i8]* @.str.3, i64 0, i64 0)
+entry:
+  switch i32 %cond, label %sw.default [
+i32 0, label %return
+ 

[PATCH] D94355: [SimplifyCFG] Add relative switch lookup tables

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem updated this revision to Diff 318372.
gulfem added a comment.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

1. Apply relative lookup table generation in fPIC mode
2. Add a test case for single value case
3. Remove hard-coding 64 bit pointers
4. Improve implementation and test coverage


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355

Files:
  clang/test/CodeGen/switch-to-lookup-table.c
  llvm/lib/Transforms/Utils/SimplifyCFG.cpp
  llvm/test/Transforms/SimplifyCFG/X86/switch_to_relative_lookup_table.ll

Index: llvm/test/Transforms/SimplifyCFG/X86/switch_to_relative_lookup_table.ll
===
--- /dev/null
+++ llvm/test/Transforms/SimplifyCFG/X86/switch_to_relative_lookup_table.ll
@@ -0,0 +1,239 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -simplifycfg -switch-to-lookup=true -keep-loops=false -S -mtriple=x86_64-unknown-linux-gnu | FileCheck %s
+; RUN: opt < %s -passes='simplify-cfg' -S -mtriple=x86_64-unknown-linux-gnu | FileCheck %s
+target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@.str = private unnamed_addr constant [5 x i8] c"zero\00", align 1
+@.str.1 = private unnamed_addr constant [4 x i8] c"one\00", align 1
+@.str.2 = private unnamed_addr constant [4 x i8] c"two\00", align 1
+@.str.3 = private unnamed_addr constant [6 x i8] c"three\00", align 1
+@.str.4 = private unnamed_addr constant [5 x i8] c"four\00", align 1
+@.str.5 = private unnamed_addr constant [5 x i8] c"five\00", align 1
+@.str.6 = private unnamed_addr constant [4 x i8] c"six\00", align 1
+@.str.7 = private unnamed_addr constant [6 x i8] c"seven\00", align 1
+@.str.8 = private unnamed_addr constant [6 x i8] c"eight\00", align 1
+@.str.9 = private unnamed_addr constant [8 x i8] c"default\00", align 1
+@.str.10 = private unnamed_addr constant [12 x i8] c"singlevalue\00", align 1
+@.str.11 = private unnamed_addr constant [9 x i8] c"default1\00", align 1
+@.str.12 = private unnamed_addr constant [9 x i8] c"default2\00", align 1
+; Relative string table lookup
+; CHECK: @switch.reltable.string_table = private unnamed_addr constant [9 x i32] [i32 trunc (i64 sub (i64 ptrtoint ([5 x i8]* @.str to i64), i64 ptrtoint ([9 x i32]* @switch.reltable.string_table to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint ([4 x i8]* @.str.1 to i64), i64 ptrtoint ([9 x i32]* @switch.reltable.string_table to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint ([4 x i8]* @.str.2 to i64), i64 ptrtoint ([9 x i32]* @switch.reltable.string_table to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint ([6 x i8]* @.str.3 to i64), i64 ptrtoint ([9 x i32]* @switch.reltable.string_table to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint ([5 x i8]* @.str.4 to i64), i64 ptrtoint ([9 x i32]* @switch.reltable.string_table to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint ([5 x i8]* @.str.5 to i64), i64 ptrtoint ([9 x i32]* @switch.reltable.string_table to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint ([4 x i8]* @.str.6 to i64), i64 ptrtoint ([9 x i32]* @switch.reltable.string_table to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint ([6 x i8]* @.str.7 to i64), i64 ptrtoint ([9 x i32]* @switch.reltable.string_table to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint ([6 x i8]* @.str.8 to i64), i64 ptrtoint ([9 x i32]* @switch.reltable.string_table to i64)) to i32)], align 4
+
+; Relative string table lookup that holes are filled with relative offset to default values
+; CHECK: @switch.reltable.string_table_holes = private unnamed_addr constant [9 x i32] [i32 trunc (i64 sub (i64 ptrtoint ([5 x i8]* @.str to i64), i64 ptrtoint ([9 x i32]* @switch.reltable.string_table_holes to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint ([8 x i8]* @.str.9 to i64), i64 ptrtoint ([9 x i32]* @switch.reltable.string_table_holes to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint ([4 x i8]* @.str.2 to i64), i64 ptrtoint ([9 x i32]* @switch.reltable.string_table_holes to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint ([6 x i8]* @.str.3 to i64), i64 ptrtoint ([9 x i32]* @switch.reltable.string_table_holes to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint ([5 x i8]* @.str.4 to i64), i64 ptrtoint ([9 x i32]* @switch.reltable.string_table_holes to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint ([5 x i8]* @.str.5 to i64), i64 ptrtoint ([9 x i32]* @switch.reltable.string_table_holes to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint ([8 x i8]* @.str.9 to i64), i64 ptrtoint ([9 x i32]* @switch.reltable.string_table_holes to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint ([6 x i8]* @.str.7 to i64), i64 ptrtoint ([9 x i32]* @switch.reltable.string_table_holes to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint ([6 x i8]* 

[PATCH] D90275: [clang][IR] Add support for leaf attribute

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

> I fixed it in
> https://reviews.llvm.org/D92493
> But that was just to make my test pass.

Ok, thank you! 
I did not realize that typo after the merge, and sorry about that!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90275

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

[PATCH] D90275: [clang][IR] Add support for leaf attribute

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

> The problem in `llvm/test/Bitcode/attributes.ll` is clear?

Instead of `define void @f70() nocallback`, it should be `define void @f69() 
nocallback`, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90275

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

[PATCH] D90275: [clang][IR] Add support for leaf attribute

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added a comment.

> This is missing a lang ref entry for `nocallback` and the `attributes.ll` 
> test is arguably broken (see below).

Could you please elaborate on missing lang ref entry? Where that should be 
added?

> The "definition" in `llvm/include/llvm/IR/Attributes.td` (see below), does 
> not match the the behavior of `clang/test/CodeGen/attr-leaf.c`.
> As I mentioned before, this doesn't have a meaning on definitions and that 
> needs to be captured in the semantics (and preferably the FE).

In that test case, `leaf` attribute is on the declaration.
What kind of a test case do you suggest to add?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90275

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

[PATCH] D90275: [clang][IR] Add support for leaf attribute

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem updated this revision to Diff 310290.
gulfem marked an inline comment as done.
gulfem added a comment.

Add a new line at the end of the test file


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90275

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-leaf.c
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-leaf.c
  llvm/include/llvm/Bitcode/LLVMBitCodes.h
  llvm/include/llvm/IR/Attributes.td
  llvm/lib/AsmParser/LLLexer.cpp
  llvm/lib/AsmParser/LLParser.cpp
  llvm/lib/AsmParser/LLToken.h
  llvm/lib/Bitcode/Reader/BitcodeReader.cpp
  llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
  llvm/lib/IR/Attributes.cpp
  llvm/lib/IR/Verifier.cpp
  llvm/lib/Transforms/Utils/CodeExtractor.cpp
  llvm/test/Bitcode/attributes.ll

Index: llvm/test/Bitcode/attributes.ll
===
--- llvm/test/Bitcode/attributes.ll
+++ llvm/test/Bitcode/attributes.ll
@@ -404,6 +404,12 @@
   ret void
 }
 
+; CHECK; define void @f69() #43
+define void @f70() nocallback
+{
+  ret void
+}
+
 ; CHECK: attributes #0 = { noreturn }
 ; CHECK: attributes #1 = { nounwind }
 ; CHECK: attributes #2 = { readnone }
@@ -446,4 +452,5 @@
 ; CHECK: attributes #39 = { sanitize_memtag }
 ; CHECK: attributes #40 = { null_pointer_is_valid }
 ; CHECK: attributes #41 = { mustprogress }
+; CHECK: attributes #42 = { nocallback }
 ; CHECK: attributes #[[NOBUILTIN]] = { nobuiltin }
Index: llvm/lib/Transforms/Utils/CodeExtractor.cpp
===
--- llvm/lib/Transforms/Utils/CodeExtractor.cpp
+++ llvm/lib/Transforms/Utils/CodeExtractor.cpp
@@ -946,6 +946,7 @@
   case Attribute::NoRecurse:
   case Attribute::InlineHint:
   case Attribute::MinSize:
+  case Attribute::NoCallback:
   case Attribute::NoDuplicate:
   case Attribute::NoFree:
   case Attribute::NoImplicitFloat:
Index: llvm/lib/IR/Verifier.cpp
===
--- llvm/lib/IR/Verifier.cpp
+++ llvm/lib/IR/Verifier.cpp
@@ -1597,6 +1597,7 @@
   case Attribute::NoReturn:
   case Attribute::NoSync:
   case Attribute::WillReturn:
+  case Attribute::NoCallback:
   case Attribute::NoCfCheck:
   case Attribute::NoUnwind:
   case Attribute::NoInline:
Index: llvm/lib/IR/Attributes.cpp
===
--- llvm/lib/IR/Attributes.cpp
+++ llvm/lib/IR/Attributes.cpp
@@ -371,6 +371,8 @@
 return "noalias";
   if (hasAttribute(Attribute::NoBuiltin))
 return "nobuiltin";
+  if (hasAttribute(Attribute::NoCallback))
+return "nocallback";
   if (hasAttribute(Attribute::NoCapture))
 return "nocapture";
   if (hasAttribute(Attribute::NoDuplicate))
Index: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
===
--- llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
+++ llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
@@ -646,6 +646,8 @@
 return bitc::ATTR_KIND_NO_ALIAS;
   case Attribute::NoBuiltin:
 return bitc::ATTR_KIND_NO_BUILTIN;
+  case Attribute::NoCallback:
+return bitc::ATTR_KIND_NO_CALLBACK;
   case Attribute::NoCapture:
 return bitc::ATTR_KIND_NO_CAPTURE;
   case Attribute::NoDuplicate:
Index: llvm/lib/Bitcode/Reader/BitcodeReader.cpp
===
--- llvm/lib/Bitcode/Reader/BitcodeReader.cpp
+++ llvm/lib/Bitcode/Reader/BitcodeReader.cpp
@@ -1433,6 +1433,8 @@
 return Attribute::NoAlias;
   case bitc::ATTR_KIND_NO_BUILTIN:
 return Attribute::NoBuiltin;
+  case bitc::ATTR_KIND_NO_CALLBACK:
+return Attribute::NoCallback;
   case bitc::ATTR_KIND_NO_CAPTURE:
 return Attribute::NoCapture;
   case bitc::ATTR_KIND_NO_DUPLICATE:
Index: llvm/lib/AsmParser/LLToken.h
===
--- llvm/lib/AsmParser/LLToken.h
+++ llvm/lib/AsmParser/LLToken.h
@@ -200,6 +200,7 @@
   kw_noalias,
   kw_noundef,
   kw_nobuiltin,
+  kw_nocallback,
   kw_nocapture,
   kw_noduplicate,
   kw_nofree,
Index: llvm/lib/AsmParser/LLParser.cpp
===
--- llvm/lib/AsmParser/LLParser.cpp
+++ llvm/lib/AsmParser/LLParser.cpp
@@ -1353,6 +1353,9 @@
   break;
 case lltok::kw_naked: B.addAttribute(Attribute::Naked); break;
 case lltok::kw_nobuiltin: B.addAttribute(Attribute::NoBuiltin); break;
+case lltok::kw_nocallback:
+  B.addAttribute(Attribute::NoCallback);
+  break;
 case lltok::kw_noduplicate: B.addAttribute(Attribute::NoDuplicate); break;
 case lltok::kw_nofree: B.addAttribute(Attribute::NoFree); break;
 case lltok::kw_noimplicitfloat:
Index: llvm/lib/AsmParser/LLLexer.cpp