In D76961#1958582, @nickdesaulniers wrote:

In D76961#1958516, @void wrote:

In D76961#1958358, @nickdesaulniers wrote:

@void would you be opposed to me posting a patch first that preprocessed llvm/test/CodeGen/X86/callbr-asm-outputs.ll with llvm/utils/update_llc_test_checks.py?

I find updating these tests with lots of checks to be a PITA, and it make it easier to maintain this test in the future and see what changed easier with my change.

Yeah, go for it.

huh, so it seems that ./llvm/utils/update_llc_test_checks.py llvm/test/CodeGen/X86/callbr-asm-outputs-pred-succ.ll doesn't actually do anything :( so I don't think I can preprocess it. I think the output needs to change; I don't understand how you write a test that update_llc_test_checks.py is happy with off of the bat.

Special talent. :-)

In D76961#1958358, @nickdesaulniers wrote:

Ok, everything is green now, kernel boots. (There's probably more verification I can do, too).

@void would you be opposed to me posting a patch first that preprocessed llvm/test/CodeGen/X86/callbr-asm-outputs.ll with llvm/utils/update_llc_test_checks.py?

I find updating these tests with lots of checks to be a PITA, and it make it easier to maintain this test in the future and see what changed easier with my change.

Friendly ping. :-)

In D75799#1950424, @xbolva00 wrote:

Please revisit patch title/description before commiting

Using a convergent attribute accomplishes the same thing and doesn't run into the issue Eli pointed out, which is that we're creating special rules for passes.

@kazu and @MaskRay I modified this to just add the "convergent" attribute to is.constant. It seems to be just fine for it. PTAL.

Good catch.

In D75799#1933603, @joerg wrote:

The original code has a functional dependency between sz and bytes and whether they can be constant evaluated. But the code doesn't express that. I don't think we can enforce that in any sensible way. There are valid use cases after all where partial inlining would result in entirely sensible decisions, just think about the more typical case of __builtin_constant_p selecting between inline asm taking immediate operands and one taking register/memory operands. That's why I am saying that I consider it a lot more useful to provide reliable building blocks to express the dependency and make sure they work.

In D75799#1932597, @joerg wrote:

Yes, builtin_expect is just noise here. The point I wanted to make is that both sz and bytes should be used in builtin_constant_p in the other condition.

In D75799#1931173, @joerg wrote:

IMO the root of the problem here is that the branches are mixing predicated variables (bytes) with non-predicated variables (sz). If users want deterministic behavior here, that shouldn't be done.

In D75799#1930254, @joerg wrote:

I was discussing this with Bill off-list to get a better idea of the original test case. Basically, with the new constant intrinsic pass, we can provide a stronger semantic: the default LLVM pass chain always constant folds expressions involving is.constant and performs DCE trivially dead branches resulting from the former folding. This means that if the original expression is adjusted from:

if (__builtin_expect(!!(sz >= 0 && sz < bytes), 0)) {
    if (!__builtin_constant_p(bytes))
    ...
}

to the functionally equivalent:

if (!(__builtin_constant_p(sz) && __builtin_constant_p(bytes) && sz >= 0 && sz >= bytes) && __builtin_expect(!!(sz >= 0 && sz < bytes), 0)) {
    if (!__builtin_constant_p(bytes))
    ...
}

then we can actually ensure proper DCE even with -O0 in the default pass chain. That depends over all on two properties:

• If __builtin_constant_p is constant folded by clang, it must DCE the appropiate places.
• the constant intrinsic pass is run With -O1 or higher, this should work in general.

In D75799#1930221, @efriedma wrote:

I'm not really happy about imposing optimization "rule", which we have to follow for correctness, without some explanation in LangRef. Hacking a specific pass without a general rule is just going to make things more messy in the future; some other pass will eventually trip over this, and we won't have any reference for how it should be fixed.

Really, to preserve the semantics the kernel wants in general, we need some notion of a "region": when we have an if statement with a builtin_constant_p condition, the body of that if statement is a region with special semantics. If we resolve a builtin_constant_p, every use of a variable used as an input to the __builtin_constant_p must be constant-folded inside that region. We can optimize within the region, we can clone the whole region, but we can't clone the entry point of the region without cloning the whole thing, and we can't merge parts of the region without merging the whole thing.

I'm not sure how to actually represent that in LLVM IR in a way that can be uniformly queried by optimizations in a reasonable way. I guess marking llvm.is.constant "convergent" partially solves the issue, but we still might have problems with optimizations that merge code.

Another friendly ping. :-)

Another friendly ping. :-)

Friendly ping. :-)

Friendly ping. :-)

Rebase and update testcase.

In D75799#1921129, @lebedev.ri wrote:

In D75799#1920869, @void wrote:

In D75799#1920392, @lebedev.ri wrote:

Can you post standalone example on godbolt that shows this link failure without this patch please?

I'm not sure what that will achieve. It would include all of the Linux kernel as part of the linking process, and it's only one file (the one I reduced the testcase from) that has the __bad_copy_from() reference still in it. Perhaps I'm missing something?

I personally still don't understand *why* after this transform we fail to DCE. Is this pass ordering issue? Heuristics issue?

In D75799#1920392, @lebedev.ri wrote:

Can you post standalone example on godbolt that shows this link failure without this patch please?

Friendly ping. :-)

s/IsUsedBy/isUsedBy/g

PTAL.

Run update_llc_test_checks.py on the testcase.

Rebase patch.

Add TCOPY to some switch statements that handle COPY

Friendly ping. :-)

In D75799#1913287, @joerg wrote:

It still seems to be a perfectly reasonable optimisation under the semantics of is.constant. The code here seems to be an abuse of it though and makes assumptions that are not sensible.

Make test more representative of what this patch is doing.

In D75799#1913130, @joerg wrote:

I'm confused by that example. If jumpthreading makes it possible to answer true, it is no differerent from inlining and other use cases. The reverse is the problematic case, turning constant cases into non-constant cases?

Reformat and test fix.

In D75799#1912044, @lebedev.ri wrote:

In D75799#1911874, @void wrote:

This is after jump threading:

Are you sure that is the correct snippet? Because if it is, i'm lost completely.

Yes. I just elided some parts that aren't super relevant. I can supply a preprocessed file though.

Alternative patch. This prevents threading blocks where a PHI node has a constant and is used by an is.constant intrinsic.

In D75799#1911594, @nikic wrote:

@void From your example, I still don't really get what the problem is / why this has to be done in JumpThreading in particular. The %6 phi that gets inserted here looks harmless to me -- the phi is trivial and will get eliminated by any simplification pass later in the pipeline (though I'm not entirely sure why a single-arg phi gets created in the first place).

What you seem to be really doing here is to move the is.constant lowering from the LowerConstantIntrinsics pass into the JumpThreading pass. It sounds to me like some kind of phase ordering issue where LowerConstantIntrinsics runs too late for the necessary DCE to be performed. Is that right? What does the IR look like at the point where the lowering is (currently) performed?

In D75799#1911541, @lebedev.ri wrote:

Doesn't fixing that instead sounds like more general fix rather than this?

In D75799#1911082, @nikic wrote:

This looks somewhat dubious to me. This seems like an InstCombine-style fold that is performed in JumpThreading -- but why? Won't it just get folded either earlier or later? As the test case is over-reduced, it's hard to tell where this would make a difference in practice.

Should be *not* matching.

Use pattern matching for intrinsic instruction matching.

• Add a verification step to ensure a COPY doesn't follow a TCOPY.
• Comment that the predicate for determining whether to use a TCOPY or COPY should be improved, but at this time we don't have enough information to determine the best criteria.

Reformat and satisfy clang-tidy.

Fix accidental change inclusion in the verifier.

Liveness is handled by the PostRA machine sink pass, so there's no need to
invalidate it. Also simplify the logic in the machine verifier that allows
stack dumps after a terminator.

Rebasing.

Don't mark TCOPY as a branch. Also s/COPY_BR/TCOPY/.

In D75098#1892171, @nickdesaulniers wrote:

Also, if INLINEASM_BR is a terminator, but we want TCOPY to be a terminator, should this patch make INLINEASM_BR NOT a terminator? Or would that be a follow up patch after creating TCOPY?

Explain what's going on in VisitGCCAsmStmt better.

Would you be okay with me submitting this and working on making INLINEASM_BR a non-terminator? I'd like to give this feature some bake time.

In D69868#1883687, @jyknight wrote:

Ugh, it's actually been that long, hasn't it...I'm really sorry about that. :(

Move lambda logic into a method. Improve some tests with additional checks.

It's been almost a month since the last comments on this review. If you need more time, please comment here. Otherwise, I will submit this with the current approvals by the end of the week.

Variable can't be 'const'

Use "auto" and change labels to clarify test.

Typo

Use the BB when creating the MBB.

Another friendly ping. :-)