LGTM. @kparzysz ?

In D84896#2188558, @hgreving wrote:

Tending to push this rel. small fix w/o an explicit Hexagon sign-off. Objections?

I'm troubleshooting a build failure that happens when this commit lands on a downstream builder. The symptom looks similar to the one reported by @aeubanks where the includes aren't present but I've ruled out the EXPERIMENTAL_LIBRARY (explicitly set OFF in the failing build).

updated diff w/context

I created this to make it easier to run builds on systems that have python3 but don't have virtualenv installed. python3 comes with venv.

In D82263#2122860, @bcain wrote:

These tests all seem to fail with

clang-11: error: invalid linker name in argument '-fuse-ld=lld'

AFAICT these tests are all using x86_64 and shouldn't be in scope for this change.

These tests all seem to fail with

Where can I learn more about the model itself and how it gets feedback? Does this mostly impact target dependent or target independent optimizations? (Sorry for hijacking the review)

simplified test case

add -march=hexagon to test case

In D77213#2005906, @kparzysz wrote:

The main changes are:

• Changed the PartwordMaskValue member functions to static non-members.
• Added LL/SC expansion of partword RMW.

Applied clang-format.

In D77213#2003730, @bcain wrote:

If I apply this patch I am still able to encounter this unreachable during a build. I can share repro details.

If I apply this patch I am still able to encounter this unreachable during a build. I can share repro details.

LGTM. Tested with Hexagon.

PIng.

Fixed the expressions in getSectionFlagString() - strings instead of chars, the nulls were the wrong choice.

In D77213#1979155, @kparzysz wrote:

Removed the phi optimization, updated tests to reflect the leftover phis.

fix indentation in getSectionFlagString per git-clang-format

Added a getSectionFlagString() function.
Changed section name delimiters in diagnostic to single-quotes

Fixed per review comments: removed superfluous parens around return expression, changed 0x0 to just 0.

removed obsolete/commented-out getIP() implementation

Fix WEAK_ALIAS() definition for hexagon

• Corrected WEAK_ALIAS definition for hexagon
• Removed .size directive from UnwindRegistersRestore.S

In D77359#1971150, @bcain wrote:

• Remove encoding == DW_EH_PE_omit guard from getEncodedP()
• Remove hexagon changes from findFDE()
• Fix hexagon unw_getcontext definition, declaration in UnwindRegistersSave.S

• Remove encoding == DW_EH_PE_omit guard from getEncodedP()
• Remove hexagon changes from findFDE()
• Fix hexagon unw_getcontext definition, declaration in UnwindRegistersSave.S

LGTM

In D77498#1968123, @sidneym wrote:

Since this isn't something that can be always known in advance I think the testcase should just be removed.

In D77498#1967870, @nathanchance wrote:

This does not take into account CLANG_DEFAULT_LINKER, resulting in a check-clang failure:

LGTM

In D76958#1958634, @bcain wrote:

In D76958#1957120, @bcain wrote:

Any general review disposition for the latest patch w/only changes to MultiSource/Benchmarks/CMakeLists.txt and cmake/modules/DetectArchitecture.c?

Any thoughts on the remainder of the current state of the patch? Anything that should prevent me from merging this as-is?

@lebedev.ri @luismarques

In D76958#1957120, @bcain wrote:

Any general review disposition for the latest patch w/only changes to MultiSource/Benchmarks/CMakeLists.txt and cmake/modules/DetectArchitecture.c?

Any general review disposition for the latest patch w/only changes to MultiSource/Benchmarks/CMakeLists.txt and cmake/modules/DetectArchitecture.c?

Omitted out-of-scope change to benchmark's cycleclock.h

This is my first change to the test-suite, let me know if I'm doing this right. :)

Accidentally omitted context lines w/previous patch

In D77098#1951149, @MaskRay wrote:

What problem does this patch solve? llvm-readobj does not depend on lib/Target/* and should not need REQUIRES: x86-registered-target

LGTM

In D75638#1917565, @bcain wrote:

@kparzysz do you have any thoughts about those review comments?

Seems like we might want a new test case for this?

In D76632#1937702, @pzheng wrote:

The build actually fails because of "#include <linux/futex.h>". Musl does not have this header file and the build fails with a missing header file error.

LGTM

This change seemed inocent enough. Were the false values here the right defaults?

In D74443#1920486, @sidneym wrote:

Fix variable name noted by: https://results.llvm-merge-guard.org/amd64_debian_testing_clang8-4524/clang-tidy.txt

@kparzysz do you have any thoughts about those review comments?

In general, I recommend qualifying metric and threshold values with their associated units: bits, bytes, pages, etc. That way it's easy to see where the unit conversions are happening and easy to see whether the logic is correct without reasoning about how the value changes among lines.

Ping @MaskRay and @sidneym -- anything left barring commit?

LGTM

I believe this test failure is unrelated to the change in question and brought it up on llvm-dev/libcxx-dev.

@MaskRay and @sidneym -- is this change ready to go or are there outstanding review items?

LGTM

Add ## for the comments in the new test cases.

Ping - any questions/suggestions about this patch?

LGTM 👍