I think I've missed an email discussing this change/deprecation: does it only affect .setAction calls? I.e. if the target only uses builders and doesn't call .setAction could it be affected by this change?

I'm pretty sure this also causes a build failure on older, but supported versions of GCC due to this bug (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56480):

../tools/llvm-tapi-diff/DiffEngine.cpp:35:71: error: specialization of template<class T, llvm::DiffAttrKind U> void llvm::DiffScalarVal<T, U>::print(llvm::raw_ostream&, std::__cxx11::string) in different namespace [-fpermissive]
                                                     std::string Indent) {
                                                                       ^

LGTM

I remember some noise due to this option (albeit on GCC), because it stops tracking indentation when the number of lines is too big, as it is for files including TableGen'ed inc files. Not sure if there is a way to improve it.

This compilation error (reproducible on GCC 6.2) is still not fixed unfortunately:

../unittests/IR/PassBuilderCallbacksTest.cpp: In member function ‘virtual void {anonymous}::LoopCallbacksTest_InstrumentedInvalidatingPasses_Test::TestBody()’:

This is mostly to get the comments on the original function and it's intent. Before this change the test would bail out on a first Instruction::Mul user which doesn't make much sense from the context. I.e. if there were two Mul users, one satisfying the SE.getSCEV(UI) == Mul condition, then whether the increment would be considered high cost would depend on the order of the Users in the users() list.
Also, it would return true (meaning high cost) if the existing instruction already computes given SCEV, which seems to be the opposite of the stated intent unless I'm misunderstanding something.

In D96536#2585609, @serge-sans-paille wrote:

Also not sure why the attempt to replace llvm:: with std:: has failed (probably some weird mix of obsolete STL with a newer patched GCC), although if it's the ABI we want, it doesn't matter.

The root of this problem lies in llvm::OptionalStorage which is specialized based on the value of llvm::is_trivially_copyable. In order to guarantee that code compiled with, say, clang, can link with LLVM compiled with, say gcc, we need to guarantee that the same specialization happen for the same type. That's the problem described in the bug https://bugs.llvm.org/show_bug.cgi?id=39427 I quoted in the commit comment.

As you point out, we cannot rely on std::is_trivially_copyable to enforce that guarantee, because it's not reproducible across compiler (this is something I didn't know back in the day, and probably hold true at some point for a subset of compiler large enough not to trigger the assert I added).
This is why we're looking for decent way to ensure llvm::is_trivially_copyable behaves thr same across compilers. And this patch is a decent minimal approach at doing so.

In D96536#2585263, @serge-sans-paille wrote:

It's better, I just still not completely get why that assert was necessary in the first place. I.e. what exact scenario is it trying to prevent? If we just want all trivially_copyable uses to be consistent across compilers, then that assert won't help (at the very least since std:: is not consistent across compiler/std versions).

Yeah, that was the original attempt: make sure that llvm::is_trivially_copyable was consistent with std::is_trivially_copyable when compiler implement it (at that time, some compiler were not supporting it all). As you know, experience proved that this was a naive approach :-/

In D96536#2582905, @serge-sans-paille wrote:

@danilaml This is certainly not complete, but that's much better than just removing an assert as proposed in https://reviews.llvm.org/D86126. Once that one is accepted, we can move forward and commit https://reviews.llvm.org/D86126

In D96553#2564017, @bkramer wrote:

In D96553#2563093, @danilaml wrote:

Don't know why no buildbot has caught it, but I think (haven't bisected yet, but this seems like the most likely culprit) this broke

LLVM :: tools/llvm-lto2/X86/pipeline.ll
lld :: ELF/lto/ltopasses-custom.ll

715dc556b782f718ce1815aaf5c58626f5fe839c should help

I don't know whether these checks are enough (have trivial copy constructor =/=> trivially copyable AFAIK), but otherwise LGTM.

Don't know why no buildbot has caught it, but I think (haven't bisected yet, but this seems like the most likely culprit) this broke

LLVM :: tools/llvm-lto2/X86/pipeline.ll
lld :: ELF/lto/ltopasses-custom.ll

So is missing an in-tree user (like AMDGPU) the only thing that prevents from merging this?

LGTM 👍

LGTM

LGTM

LGTM

LGTM

LGTM

Why does the cache cause this error in mypy? Is it a documented problem?

Landed in https://github.com/llvm/llvm-lnt/commit/e736349f7cca1b18e93b6a12e222db0bafcf54f5

Landed in https://github.com/llvm/llvm-lnt/commit/2e93cbd17fd5eb4b2e3773e5022186feac554d7c

Committed in https://github.com/llvm/llvm-lnt/commit/0982122b76efad15e244a18b42ce77de2a906140

LGTM

Sorry, I'm not aware of a buildbot for LNT. I did not even know there was one before.

I'm personally more enclined to prevent lit from running without lit installed because then some of the tests are skipped.

In D94418#2498606, @thopre wrote:

In D94418#2498419, @danilaml wrote:

In D94418#2498383, @thopre wrote:

In D94418#2498376, @danilaml wrote:

Sorry, pushed to the wrong branch (Phabricator/arcanist workflow is annoying >_<) and didn't notice your comment.

Hm, I've made sure that all tests pass on both py2 and py3 before submitting and I can't reproduce it on my machine.
Could you check if test_suite-profile-import.py.tmp.err and test_suite-profile-import.py.tmp.log have any suspicious errors (they should be in <..>/test_run_tmp/runtest/Output/)?

Do you use tox to run the tests?

I use lit directly (lit -svv ./tests) which what I understand tox does as well. I've rerun all tests using tox and the only test that failed was server/ui/V4Pages.py (I believe it's because I've already had an lnt instance running and that test doesn't account for that possibility).

Are you testing your patch on top of 042938b5c332fa63f1fefdd7116f39e161b8c51b? My guess is you are testing on a commit prior to f98bc1b3018248fbe551b0e7491ac5ba388216d5 which prevents tests involving profile from being run. Btw, using tox with a comparison of before/after logs allows you to spot codestyle issues added by your patch like the double glob import I pointed out.

Best regards,

Thomas

ping?

In D94418#2498383, @thopre wrote:

In D94418#2498376, @danilaml wrote:

Sorry, pushed to the wrong branch (Phabricator/arcanist workflow is annoying >_<) and didn't notice your comment.

Hm, I've made sure that all tests pass on both py2 and py3 before submitting and I can't reproduce it on my machine.
Could you check if test_suite-profile-import.py.tmp.err and test_suite-profile-import.py.tmp.log have any suspicious errors (they should be in <..>/test_run_tmp/runtest/Output/)?

Do you use tox to run the tests?

I use lit directly (lit -svv ./tests) which what I understand tox does as well. I've rerun all tests using tox and the only test that failed was server/ui/V4Pages.py (I believe it's because I've already had an lnt instance running and that test doesn't account for that possibility).

Sorry, pushed to the wrong branch (Phabricator/arcanist workflow is annoying >_<) and didn't notice your comment.

In D94418#2497746, @thopre wrote:

Could you explain me what does the test you added do? I'm not familiar with lnt runtest and its option nor with all the lit thingy.

Fixed tests.

Added a test.

I'll try to come up with a test case. I thought there were already tests for profile data, so I need to look into why they didn't catch it.

In D75746#2475681, @lebedev.ri wrote:

Is there no transform that can be taught to do this cleanup instead?

@TOCK
I've meant the surrounding context for the diff (https://llvm.org/docs/Phabricator.html#phabricator-request-review-web),
i.e. git show HEAD -U999999

The diffs lack context.

I've taken a liberty of adding more reviewers to hopefully help get this going.

Ping?

Rebased.
Had some time to come back to this.
I've tried implementing the following proposed way to get VTC locally:

Committed in 4c57b004f148dffc6db199890d91fc3e6e578972

Btw, it looks like lnt's .arcconfig is missing a callsign which broke moz-phab when I tried to submit using it.

In D62574#2135662, @ebevhan wrote:

It's generally not safe to alter address spaces below the top level. C is just very permissive about it.

What are the remaining roadblocks left before this patch can be merged? I'm interested in having a target-specific way to define the allowed explicit/implicit address space conversions.
Also, it appears that currently whether implicit casts between pointers of different AS are allowed is determined by the superset relations only, however https://reviews.llvm.org/D73360 introduced another (openCL-specific) variable into the mix: whether the conversion is a top level or not. IMHO, this should also be configured by the target, although I'm not sure whether it needs a separate hook (isBitcastNoop or similar?) or it needs to check the legality of the implicit casts differently.

ping

@RKSimon sadly not.

@dblaikie It was actually failing on some of the lit tests when compiler was compiled with EXPENSIVE_CHECKS enabled (kind of how I discovered it), but after this recent refactor which removed a sort step: https://github.com/llvm/llvm-project/commit/ed766f1bb1040a520fb5646ab75851e2b0fd66df#diff-7b43e801c5abbb03d571f9aad6af63a5L792
I don't know if the codepath with ::sort (which has those checks) can be hit from llvm IR. Instead, I've added an explicit check to AttributesTest.cpp that currently asserts.

Sorry for commenting on an old merge PR, but was looking through some asserts and stumbled on this code which raised some questions.

Updated revision with additional checks and rebased.