[Lldb-commits] [PATCH] D41962: Fix TestYMMRegisters for older machines without AVX2

[Roman Lebedev via Phabricator via lldb-commits]

lebedev.ri added inline comments.



Comment at: 
packages/Python/lldbsuite/test/functionalities/register/intel_avx/main.c:21
+  static volatile unsigned haveAVX2;
+  haveAVX2 = __builtin_cpu_supports("avx2");
   unsigned int ymmallones = 0x;

Note that you need to call `__builtin_cpu_init()` before calling 
`__builtin_cpu_supports()`.
Or maybe it is already called before this?


Repository:
  rL LLVM

https://reviews.llvm.org/D41962



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

[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

[Roman Lebedev via Phabricator via lldb-commits]

lebedev.ri added a comment.

In D56322#1389013 , @labath wrote:

> Btw, I've just noticed that the files you've added here still have the old 
> license header.


Would be good to get at least an automatic Herald rule for this
I suspect there might be more occurrences of patches/diffs/commits with old 
license in the time to come.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56322



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

[Lldb-commits] [PATCH] D59539: [llvm-exegesis] Option to lobotomize dbscan (PR40880)

[Roman Lebedev via Phabricator via lldb-commits]

lebedev.ri created this revision.
lebedev.ri added a reviewer: courbet.
lebedev.ri added a project: LLVM.
Herald added subscribers: lldb-commits, jdoerfert, abidh, tschuett.
Herald added a project: LLDB.

Let's suppose we have measured 4 different opcodes, and got: `0.5`, `1.0`, 
`1.5`, `2.0`.
Let's suppose we are using `-analysis-clustering-epsilon=0.5`.
By default now we will start processing the `0.5` point, find that `1.0` is 
it's neighbor, add them to a new cluster.
Then we will notice that `1.5` is a neighbor of `1.0` and add it to that same 
cluster.
Then we will notice that `2.0` is a neighbor of `1.5` and add it to that same 
cluster.
So all these points ended up in the same cluster.
This may or may not be a correct implementation of dbscan clustering algorithm.

But this is rather horribly broken for the reasons of comparing the clusters 
with the LLVM sched data.
Let's suppose all those opcodes are currently in the same sched cluster.
If i specify `-analysis-inconsistency-epsilon=0.5`, then no matter
the LLVM values this cluster will **never** match the LLVM values,
and thus this cluster will **always** be displayed as inconsistent.

The solution is obviously to split off some of these opcodes into different 
sched cluster.
But how do i do that? Out of 4 opcodes displayed in the inconsistency report,
which ones are the "bad ones"? Which ones are the most different from the 
checked-in data?
I'd need to go in to the `.yaml` and look it up manually.

The trivial solution is to, when creating clusters, don't use the full dbscan 
algorithm,
but instead "pick some unclustered point, pick all unclustered points that are 
it's neighbor,
put them all into a new cluster, repeat". And just so as it happens, we can 
arrive
at that algorithm by not performing the "add neighbors of a neighbor to the 
cluster" step.

(This will also help with opcode denoising/stabilization)

While the current default is good for abstract 'analyse clustering of 
measurements',
i'm not sure how often that is the actual goal, not 'compare llvm data with 
measurements'.
So i'm not sure what should be the default.

Thoughts?

This is //yet another// step to bring me closer to being able to continue 
cleanup of bdver2 sched model..

Fixes PR40880 .


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D59539

Files:
  docs/CommandGuide/llvm-exegesis.rst
  
test/tools/llvm-exegesis/X86/analysis-same-cluster-for-ops-in-different-sched-clusters.test
  test/tools/llvm-exegesis/X86/analysis-simplified-dbscan.test
  tools/llvm-exegesis/lib/Clustering.cpp
  tools/llvm-exegesis/lib/Clustering.h
  tools/llvm-exegesis/llvm-exegesis.cpp

Index: tools/llvm-exegesis/llvm-exegesis.cpp
===
--- tools/llvm-exegesis/llvm-exegesis.cpp
+++ tools/llvm-exegesis/llvm-exegesis.cpp
@@ -66,7 +66,7 @@
   cl::cat(Options), cl::init(""));
 
 static cl::opt BenchmarkMode(
-"mode", cl::desc("the mode to run"), cl::cat(BenchmarkOptions),
+"mode", cl::desc("the mode to run"), cl::cat(Options),
 cl::values(clEnumValN(exegesis::InstructionBenchmark::Latency, "latency",
   "Instruction Latency"),
clEnumValN(exegesis::InstructionBenchmark::InverseThroughput,
@@ -99,6 +99,14 @@
 cl::desc("dbscan epsilon for benchmark point clustering"),
 cl::cat(AnalysisOptions), cl::init(0.1));
 
+static cl::opt AnalysisSimplifiedDbscan(
+"analysis-simplified-dbscan",
+cl::desc("by default, full dbscan algo is used to create clusters. this "
+ "option cripples the algo to not add the neighbors of the "
+ "neighboring point being analysed to the cluster. this results in "
+ "smaller, more stable clusters"),
+cl::cat(AnalysisOptions), cl::init(false));
+
 static cl::opt AnalysisInconsistencyEpsilon(
 "analysis-inconsistency-epsilon",
 cl::desc("epsilon for detection of when the cluster is different from the "
@@ -461,7 +469,7 @@
 
   const auto Clustering = ExitOnErr(InstructionBenchmarkClustering::create(
   Points, AnalysisNumPoints, AnalysisClusteringEpsilon,
-  InstrInfo->getNumOpcodes()));
+  AnalysisSimplifiedDbscan, InstrInfo->getNumOpcodes()));
 
   const Analysis Analyzer(*TheTarget, std::move(InstrInfo), Clustering,
   AnalysisInconsistencyEpsilon,
Index: tools/llvm-exegesis/lib/Clustering.h
===
--- tools/llvm-exegesis/lib/Clustering.h
+++ tools/llvm-exegesis/lib/Clustering.h
@@ -30,6 +30,7 @@
   static llvm::Expected
   create(const std::vector &Points, size_t MinPts,
  double AnalysisClusteringEpsilon,
+ bool AnalysisSimplifiedDbscan = false,
  llvm::Optional NumOpcodes = llvm::None);
 
   class ClusterId {
@@ -117,7 +118,7 @@
 private:
   InstructionBenchmarkClustering(
   const std::vector &Points,
-  doubl

[Lldb-commits] [PATCH] D130586: [cmake] Use `CMAKE_INSTALL_LIBDIR` too

[Roman Lebedev via Phabricator via lldb-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.
Herald added a subscriber: StephenFan.

What's the status here? Should this be abandoned?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130586

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

[Lldb-commits] [PATCH] D102942: Remove or use variables which are unused but set.

[Roman Lebedev via Phabricator via lldb-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.

(removing from my queue - i don't expect to provide any review here)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102942

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

[Lldb-commits] [PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Roman Lebedev via Phabricator via lldb-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.
This revision is now accepted and ready to land.

Removing from queue - i don't expect to review this.
Looks like this has been reverted twice now, presumably llvm stage 2/linux 
kernel/chrome
should be enough of a coverage to be sure there isn't any obvious 
false-positives this time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100581

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

[Lldb-commits] [PATCH] D98179: [lit] Sort test start times based on prior test timing data

[Roman Lebedev via Phabricator via lldb-commits]

lebedev.ri added a comment.

@nlopes are you talking about sorting *all* lit output, or the summary?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98179

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

[Lldb-commits] [PATCH] D98179: [lit] Sort test start times based on prior test timing data

[Roman Lebedev via Phabricator via lldb-commits]

lebedev.ri added a comment.

In D98179#2639476 , @nlopes wrote:

> I'm talking about sorting just the summary of failed tests, not the whole 
> output. We need the whole -vv output, but that can be out of order.

Aha! +1 then, i think sorting summary should be both easy and nice to have.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98179

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

[Lldb-commits] [PATCH] D99484: Use `GNUInstallDirs` to support custom installation dirs.

[Roman Lebedev via Phabricator via lldb-commits]

lebedev.ri added a comment.

Thank you!
The changes here look about reasonable to me.
I have not checked if there are some changes that were missed.
Usage of `CMAKE_INSTALL_FULL_` is suspect to me. Are you sure those are correct?




Comment at: compiler-rt/cmake/Modules/AddCompilerRT.cmake:513
   install(FILES ${file_name}
-DESTINATION ${COMPILER_RT_INSTALL_PATH}/share
+DESTINATION ${COMPILER_RT_INSTALL_PATH}/${CMAKE_INSTALL_FULL_INCLUDEDIR}
 COMPONENT ${component})

This looks suspect



Comment at: compiler-rt/cmake/Modules/AddCompilerRT.cmake:530
 PERMISSIONS OWNER_READ OWNER_WRITE OWNER_EXECUTE GROUP_READ GROUP_EXECUTE 
WORLD_READ WORLD_EXECUTE
-DESTINATION ${COMPILER_RT_INSTALL_PATH}/bin)
+DESTINATION ${COMPILER_RT_INSTALL_PATH}/${CMAKE_INSTALL_FULL_BINDIR})
 endmacro(add_compiler_rt_script src name)

This looks suspect



Comment at: compiler-rt/cmake/Modules/CompilerRTDarwinUtils.cmake:511
 set(DARWIN_macho_embedded_LIBRARY_INSTALL_DIR
-  ${COMPILER_RT_INSTALL_PATH}/lib/macho_embedded)
+  ${COMPILER_RT_INSTALL_PATH}/${CMAKE_INSTALL_FULL_LIBDIR}/macho_embedded)
   

This looks suspect



Comment at: compiler-rt/cmake/Modules/CompilerRTUtils.cmake:389
 get_compiler_rt_target(${arch} target)
-set(${install_dir} ${COMPILER_RT_INSTALL_PATH}/lib/${target} PARENT_SCOPE)
+set(${install_dir} 
${COMPILER_RT_INSTALL_PATH}/${CMAKE_INSTALL_FULL_LIBDIR}/${target} PARENT_SCOPE)
   else()

This looks suspect



Comment at: compiler-rt/cmake/base-config-ix.cmake:100
   set(COMPILER_RT_LIBRARY_INSTALL_DIR
-${COMPILER_RT_INSTALL_PATH}/lib/${COMPILER_RT_OS_DIR})
+
${COMPILER_RT_INSTALL_PATH}/${CMAKE_INSTALL_FULL_LIBDIR}/${COMPILER_RT_OS_DIR})
 endif()

This looks suspect



Comment at: compiler-rt/include/CMakeLists.txt:72-87
+  DESTINATION 
${COMPILER_RT_INSTALL_PATH}/${CMAKE_INSTALL_FULL_INCLUDEDIR}/sanitizer)
 # Install fuzzer headers.
 install(FILES ${FUZZER_HEADERS}
   COMPONENT compiler-rt-headers
   PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ
-  DESTINATION ${COMPILER_RT_INSTALL_PATH}/include/fuzzer)
+  DESTINATION 
${COMPILER_RT_INSTALL_PATH}/${CMAKE_INSTALL_FULL_INCLUDEDIR}/fuzzer)
 # Install xray headers.

This looks suspect



Comment at: compiler-rt/lib/dfsan/CMakeLists.txt:64-65
 add_dependencies(dfsan dfsan_abilist)
 install(FILES ${dfsan_abilist_filename}
-DESTINATION ${COMPILER_RT_INSTALL_PATH}/share)
+DESTINATION ${COMPILER_RT_INSTALL_PATH}/${CMAKE_INSTALL_FULL_DATADIR})

This looks suspect



Comment at: polly/cmake/CMakeLists.txt:85
+set(POLLY_CONFIG_CMAKE_DIR 
"${POLLY_INSTALL_PREFIX}${CMAKE_INSTALL_PREFIX}/${POLLY_INSTALL_PACKAGE_DIR}")
+set(POLLY_CONFIG_LIBRARY_DIRS 
"${POLLY_INSTALL_PREFIX}${CMAKE_INSTALL_FULL_LIBDIR}${LLVM_LIBDIR_SUFFIX}")
 if (POLLY_BUNDLED_ISL)

This looks suspect


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99484

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

[Lldb-commits] [PATCH] D98179: [lit] Sort test start times based on prior test timing data

[Roman Lebedev via Phabricator via lldb-commits]

lebedev.ri added a comment.

In D98179#2689075 , @mstorsjo wrote:

> Something related to the time recording seems to fail intermittently on 
> buildbots: https://lab.llvm.org/buildbot#builders/93/builds/2697

I'll cut to the fix here: @davezarzycki is there any reason why the timing data 
is stored in a CSV format, and not JSON.
The fix is to switch to latter.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98179

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

[Lldb-commits] [PATCH] D63370: Specify log level for CMake messages (less stderr)

[Roman Lebedev via Phabricator via lldb-commits]

lebedev.ri added a comment.

The `llvm/utils/benchmark/` changes - can you please either submit them 
upstream yourself, or explicitly state that it is okay to **steal** them from 
you and submit?


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

https://reviews.llvm.org/D63370



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

[Lldb-commits] [PATCH] D63370: Specify log level for CMake messages (less stderr)

[Roman Lebedev via Phabricator via lldb-commits]

lebedev.ri added a comment.

In D63370#1544970 , @siedentop wrote:

> In D63370#1544767 , @lebedev.ri 
> wrote:
>
> > The `llvm/utils/benchmark/` changes - can you please either submit them 
> > upstream yourself, or explicitly state that it is okay to **steal** them 
> > from you and submit?
>
>
> Thanks, @lebedev.ri, I was not aware that `llvm/utils/benchmark` contains 
> Google Benchmark.
>
> I just checked: all three modified files in  `llvm/utils/benchmark/`  already 
> have the same modifications on upstream (HEAD == 090faecb454fb).


Ah yes, apparently so, thanks for checking!
I have no further comments then.

> I propose that I remove my modifications from this review. I'll go ahead with 
> this -- but please let me know if you disagree.




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

https://reviews.llvm.org/D63370



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

[Lldb-commits] [PATCH] D89813: [CMake][NFC] Limit the use of uppercase_CMAKE_BUILD_TYPE

[Roman Lebedev via Phabricator via lldb-commits]

lebedev.ri requested changes to this revision.
lebedev.ri added inline comments.
This revision now requires changes to proceed.



Comment at: libcxx/utils/google-benchmark/test/CMakeLists.txt:8-9
 # strip -DNDEBUG from the default CMake flags in DEBUG mode.
-string(TOUPPER "${CMAKE_BUILD_TYPE}" uppercase_CMAKE_BUILD_TYPE)
-if( NOT uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG" )
   add_definitions( -UNDEBUG )

How is this an unnecessary usage?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89813

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