Page MenuHomePhabricator

void (Bill Wendling)
Mind Taker

Projects

User does not belong to any projects.

User Details

User Since
Dec 3 2012, 10:22 AM (383 w, 2 d)

Sent here from the planet Zvddw, Bill's mission is to conquer the world by staying at home and occasionally (read: always) playing poker.

In his spare time, he works on LLVM-related things.

Recent Activity

Mon, Apr 6

void accepted D77600: [CallSite Removal] a CallBase is never an IndirectCall for isInlineAsm.
Mon, Apr 6, 3:48 PM · Restricted Project

Fri, Apr 3

void accepted D77356: [test] preformat test with update_llc_test_checks.py NFC.
Fri, Apr 3, 12:25 PM · Restricted Project

Thu, Apr 2

void added a comment to D76961: [SelectionDAG] fix predecessor list for INLINEASM_BRs' parent.

@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. :-)

Thu, Apr 2, 8:04 PM · Restricted Project
void added a comment to D76961: [SelectionDAG] fix predecessor list for INLINEASM_BRs' parent.

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.

Thu, Apr 2, 5:54 PM · Restricted Project

Tue, Mar 31

void updated subscribers of D62627: [NFC] Do not run CGProfilePass when not using integrated assembler.

Friendly ping. :-)

Tue, Mar 31, 2:42 AM · Restricted Project, Restricted Project

Mon, Mar 30

void committed rGfa496ce3c677: [Intrinsic] Give "is.constant" the "convergent" attribute (authored by void).
[Intrinsic] Give "is.constant" the "convergent" attribute
Mon, Mar 30, 11:58 AM
void closed D75799: [Intrinsic] Give "is.constant" the "convergent" attribute.
Mon, Mar 30, 11:58 AM · Restricted Project
void retitled D75799: [Intrinsic] Give "is.constant" the "convergent" attribute from [JumpThreading] Don't create PHI nodes with "is.constant" values to [Intrinsic] Give "is.constant" the "convergent" attribute.
Mon, Mar 30, 11:56 AM · Restricted Project
void added a comment to D75799: [Intrinsic] Give "is.constant" the "convergent" attribute.

Please revisit patch title/description before commiting

Mon, Mar 30, 11:56 AM · Restricted Project

Sun, Mar 29

void updated the diff for D75799: [Intrinsic] Give "is.constant" the "convergent" attribute.

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.

Sun, Mar 29, 9:24 PM · Restricted Project
void added a comment to D75799: [Intrinsic] Give "is.constant" the "convergent" attribute.

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

Sun, Mar 29, 9:24 PM · Restricted Project
void added inline comments to D73422: [IR] Delete MODULE_CODE_DEPLIB.
Sun, Mar 29, 7:48 PM · Restricted Project

Fri, Mar 27

void accepted D76961: [SelectionDAG] fix predecessor list for INLINEASM_BRs' parent.

Good catch.

Fri, Mar 27, 6:12 PM · Restricted Project

Thu, Mar 26

void added inline comments to D75799: [Intrinsic] Give "is.constant" the "convergent" attribute.
Thu, Mar 26, 1:35 AM · Restricted Project

Tue, Mar 24

void added reviewers for D75799: [Intrinsic] Give "is.constant" the "convergent" attribute: kazu, MaskRay.
Tue, Mar 24, 4:30 PM · Restricted Project

Mon, Mar 23

void added inline comments to D76206: [gcov] Fix simultaneous .gcda creation.
Mon, Mar 23, 12:33 PM · Restricted Project

Fri, Mar 20

void added inline comments to D74809: [MBP][X86] Include static prof data when collecting loop BBs.
Fri, Mar 20, 3:45 PM · Restricted Project
void added a comment to D75799: [Intrinsic] Give "is.constant" the "convergent" attribute.

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.

Fri, Mar 20, 1:01 PM · Restricted Project

Thu, Mar 19

void added a comment to D75799: [Intrinsic] Give "is.constant" the "convergent" attribute.

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.

Thu, Mar 19, 9:54 PM · Restricted Project
void added a comment to D75799: [Intrinsic] Give "is.constant" the "convergent" attribute.

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.

Thu, Mar 19, 12:32 PM · Restricted Project

Wed, Mar 18

void added a comment to D75799: [Intrinsic] Give "is.constant" the "convergent" attribute.

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.
Wed, Mar 18, 7:00 PM · Restricted Project
void added a comment to D75799: [Intrinsic] Give "is.constant" the "convergent" attribute.

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.

Wed, Mar 18, 7:00 PM · Restricted Project
void added a comment to D75098: Add TCOPY, a terminator form of the COPY instr.

Another friendly ping. :-)

Wed, Mar 18, 3:13 PM · Restricted Project
void added a comment to D75799: [Intrinsic] Give "is.constant" the "convergent" attribute.

Another friendly ping. :-)

Wed, Mar 18, 3:13 PM · Restricted Project

Sun, Mar 15

void added a comment to D75799: [Intrinsic] Give "is.constant" the "convergent" attribute.

Friendly ping. :-)

Sun, Mar 15, 3:32 PM · Restricted Project
void added a comment to D75098: Add TCOPY, a terminator form of the COPY instr.

Friendly ping. :-)

Sun, Mar 15, 3:32 PM · Restricted Project
void updated the diff for D75098: Add TCOPY, a terminator form of the COPY instr.

Rebase and update testcase.

Sun, Mar 15, 3:32 PM · Restricted Project

Fri, Mar 13

void added a comment to D75799: [Intrinsic] Give "is.constant" the "convergent" attribute.

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?

Fri, Mar 13, 5:06 AM · Restricted Project

Thu, Mar 12

void added a comment to D75799: [Intrinsic] Give "is.constant" the "convergent" attribute.

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

Thu, Mar 12, 7:04 PM · Restricted Project
void added a comment to D75799: [Intrinsic] Give "is.constant" the "convergent" attribute.

Friendly ping. :-)

Thu, Mar 12, 2:07 PM · Restricted Project
void updated the diff for D75799: [Intrinsic] Give "is.constant" the "convergent" attribute.

s/IsUsedBy/isUsedBy/g

Thu, Mar 12, 2:07 PM · Restricted Project

Wed, Mar 11

void committed rG6aebf0ee56e5: Specify branch probabilities for callbr dests (authored by void).
Specify branch probabilities for callbr dests
Wed, Mar 11, 9:07 PM
void closed D72656: Specify branch probabilities for callbr dests.
Wed, Mar 11, 9:07 PM · Restricted Project
void added a comment to D72656: Specify branch probabilities for callbr dests.

PTAL.

Wed, Mar 11, 6:20 PM · Restricted Project
void updated the diff for D72656: Specify branch probabilities for callbr dests.

Run update_llc_test_checks.py on the testcase.

Wed, Mar 11, 6:20 PM · Restricted Project
void added inline comments to D72656: Specify branch probabilities for callbr dests.
Wed, Mar 11, 6:10 PM · Restricted Project
void updated the diff for D72656: Specify branch probabilities for callbr dests.

Rebase patch.

Wed, Mar 11, 6:10 PM · Restricted Project

Tue, Mar 10

void added inline comments to D75098: Add TCOPY, a terminator form of the COPY instr.
Tue, Mar 10, 5:00 PM · Restricted Project
void committed rG218dd339541f: Add triple for non-x86 environments. (authored by void).
Add triple for non-x86 environments.
Tue, Mar 10, 3:54 PM
void committed rG72aa619a7fe0: Warn of uninitialized variables on asm goto's indirect branch (authored by void).
Warn of uninitialized variables on asm goto's indirect branch
Tue, Mar 10, 2:16 PM
void closed D71314: Warn of uninitialized variables on asm goto's indirect branch.
Tue, Mar 10, 2:15 PM · Restricted Project
void updated the diff for D75098: Add TCOPY, a terminator form of the COPY instr.

Add TCOPY to some switch statements that handle COPY

Tue, Mar 10, 1:39 PM · Restricted Project
void added a comment to D71314: Warn of uninitialized variables on asm goto's indirect branch.

Friendly ping. :-)

Tue, Mar 10, 5:48 AM · Restricted Project

Mar 9 2020

void added a comment to D75799: [Intrinsic] Give "is.constant" the "convergent" attribute.

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.

Mar 9 2020, 2:36 PM · Restricted Project
void updated the diff for D75799: [Intrinsic] Give "is.constant" the "convergent" attribute.

Make test more representative of what this patch is doing.

Mar 9 2020, 2:35 PM · Restricted Project
void added a comment to D75799: [Intrinsic] Give "is.constant" the "convergent" attribute.

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?

Mar 9 2020, 1:30 PM · Restricted Project
void updated the diff for D75799: [Intrinsic] Give "is.constant" the "convergent" attribute.

Reformat and test fix.

Mar 9 2020, 12:25 PM · Restricted Project
void added a comment to D75799: [Intrinsic] Give "is.constant" the "convergent" attribute.

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.

Mar 9 2020, 12:25 PM · Restricted Project
void updated the diff for D75799: [Intrinsic] Give "is.constant" the "convergent" attribute.

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

Mar 9 2020, 2:07 AM · Restricted Project

Mar 8 2020

void added a comment to D75799: [Intrinsic] Give "is.constant" the "convergent" attribute.

@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?

Mar 8 2020, 6:38 PM · Restricted Project
void added a comment to D75799: [Intrinsic] Give "is.constant" the "convergent" attribute.

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

Mar 8 2020, 12:33 AM · Restricted Project

Mar 7 2020

void added a comment to D75799: [Intrinsic] Give "is.constant" the "convergent" attribute.

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.

Mar 7 2020, 4:45 PM · Restricted Project
void updated the diff for D75799: [Intrinsic] Give "is.constant" the "convergent" attribute.

Should be *not* matching.

Mar 7 2020, 3:48 AM · Restricted Project
void updated the diff for D75799: [Intrinsic] Give "is.constant" the "convergent" attribute.

Use pattern matching for intrinsic instruction matching.

Mar 7 2020, 3:15 AM · Restricted Project
void created D75799: [Intrinsic] Give "is.constant" the "convergent" attribute.
Mar 7 2020, 1:06 AM · Restricted Project

Mar 2 2020

void added inline comments to D75098: Add TCOPY, a terminator form of the COPY instr.
Mar 2 2020, 3:39 PM · Restricted Project
void updated the diff for D75098: Add TCOPY, a terminator form of the COPY instr.
  • 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.
Mar 2 2020, 1:43 PM · Restricted Project

Feb 27 2020

void added inline comments to D75098: Add TCOPY, a terminator form of the COPY instr.
Feb 27 2020, 4:11 PM · Restricted Project
void updated the diff for D75098: Add TCOPY, a terminator form of the COPY instr.

Reformat and satisfy clang-tidy.

Feb 27 2020, 3:07 PM · Restricted Project
void added inline comments to D75098: Add TCOPY, a terminator form of the COPY instr.
Feb 27 2020, 2:41 PM · Restricted Project
void updated the diff for D75098: Add TCOPY, a terminator form of the COPY instr.

Fix accidental change inclusion in the verifier.

Feb 27 2020, 1:59 PM · Restricted Project
void updated the diff for D75098: Add TCOPY, a terminator form of the COPY instr.

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.

Feb 27 2020, 1:30 PM · Restricted Project

Feb 26 2020

void updated the diff for D71314: Warn of uninitialized variables on asm goto's indirect branch.

Rebasing.

Feb 26 2020, 3:42 PM · Restricted Project
void retitled D71314: Warn of uninitialized variables on asm goto's indirect branch from Emit a warning if a variable is uninitialized in indirect ASM goto destination. to Warn of uninitialized variables on asm goto's indirect branch.
Feb 26 2020, 3:42 PM · Restricted Project

Feb 25 2020

void added inline comments to D75098: Add TCOPY, a terminator form of the COPY instr.
Feb 25 2020, 5:57 PM · Restricted Project
void retitled D75098: Add TCOPY, a terminator form of the COPY instr from Add COPY_BR, a terminator form of the COPY instr to Add TCOPY, a terminator form of the COPY instr.
Feb 25 2020, 5:57 PM · Restricted Project
void updated the diff for D75098: Add TCOPY, a terminator form of the COPY instr.

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

Feb 25 2020, 5:48 PM · Restricted Project
void added a comment to D75098: Add TCOPY, a terminator form of the COPY instr.

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?

Feb 25 2020, 3:08 PM · Restricted Project
void added inline comments to D75098: Add TCOPY, a terminator form of the COPY instr.
Feb 25 2020, 1:11 PM · Restricted Project
void committed rG6d0d1a63f2a6: Use "nop" to avoid size warnings. (authored by void).
Use "nop" to avoid size warnings.
Feb 25 2020, 12:33 PM
void committed rGe11f9fb45085: Add 'l' constraint to goto label reference (authored by void).
Add 'l' constraint to goto label reference
Feb 25 2020, 11:52 AM

Feb 24 2020

void updated the diff for D71314: Warn of uninitialized variables on asm goto's indirect branch.

Explain what's going on in VisitGCCAsmStmt better.

Feb 24 2020, 7:10 PM · Restricted Project
void added inline comments to D71314: Warn of uninitialized variables on asm goto's indirect branch.
Feb 24 2020, 7:10 PM · Restricted Project
void committed rG50cac248773c: Support output constraints on "asm goto" (authored by void).
Support output constraints on "asm goto"
Feb 24 2020, 7:01 PM
void closed D69876: Support output constraints on "asm goto".
Feb 24 2020, 7:01 PM · Restricted Project, Restricted Project
void created D75098: Add TCOPY, a terminator form of the COPY instr.
Feb 24 2020, 6:52 PM · Restricted Project
void committed rG23c2a5ce33f0: Allow "callbr" to return non-void values (authored by void).
Allow "callbr" to return non-void values
Feb 24 2020, 6:34 PM
void closed D69868: Allow "callbr" to return non-void values.
Feb 24 2020, 6:34 PM · Restricted Project, Restricted Project
void added inline comments to D69868: Allow "callbr" to return non-void values.
Feb 24 2020, 4:36 PM · Restricted Project, Restricted Project

Feb 21 2020

void updated the summary of D69868: Allow "callbr" to return non-void values.
Feb 21 2020, 3:16 PM · Restricted Project, Restricted Project

Feb 20 2020

void committed rG2fe457690da0: Filter callbr insts from critical edge splitting (authored by void).
Filter callbr insts from critical edge splitting
Feb 20 2020, 4:28 PM

Feb 19 2020

void added a comment to D69868: Allow "callbr" to return non-void values.

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.

Feb 19 2020, 5:47 PM · Restricted Project, Restricted Project
void added a comment to D69868: Allow "callbr" to return non-void values.

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

Feb 19 2020, 2:55 PM · Restricted Project, Restricted Project
void retitled D69876: Support output constraints on "asm goto" from Allow output constraints on "asm goto" to Support output constraints on "asm goto".
Feb 19 2020, 2:36 PM · Restricted Project, Restricted Project
void committed rG129c911efaa4: Include static prof data when collecting loop BBs (authored by void).
Include static prof data when collecting loop BBs
Feb 19 2020, 11:37 AM
void closed D74809: [MBP][X86] Include static prof data when collecting loop BBs.
Feb 19 2020, 11:37 AM · Restricted Project
void updated the diff for D74809: [MBP][X86] Include static prof data when collecting loop BBs.

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

Feb 19 2020, 10:37 AM · Restricted Project
void added inline comments to D74809: [MBP][X86] Include static prof data when collecting loop BBs.
Feb 19 2020, 10:37 AM · Restricted Project

Feb 18 2020

void added a comment to D69868: Allow "callbr" to return non-void values.

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.

Feb 18 2020, 9:55 PM · Restricted Project, Restricted Project
void added inline comments to D74809: [MBP][X86] Include static prof data when collecting loop BBs.
Feb 18 2020, 9:11 PM · Restricted Project
void added reviewers for D74809: [MBP][X86] Include static prof data when collecting loop BBs: yamauchi, nickdesaulniers.
Feb 18 2020, 5:27 PM · Restricted Project
void created D74809: [MBP][X86] Include static prof data when collecting loop BBs.
Feb 18 2020, 5:27 PM · Restricted Project
void added a reviewer for D69868: Allow "callbr" to return non-void values: lattner.
Feb 18 2020, 4:59 PM · Restricted Project, Restricted Project

Feb 17 2020

void updated the diff for D71314: Warn of uninitialized variables on asm goto's indirect branch.

Variable can't be 'const'

Feb 17 2020, 6:12 PM · Restricted Project

Feb 15 2020

void updated the diff for D71314: Warn of uninitialized variables on asm goto's indirect branch.

Use "auto" and change labels to clarify test.

Feb 15 2020, 2:19 PM · Restricted Project
void updated the diff for D69876: Support output constraints on "asm goto".

Typo

Feb 15 2020, 2:10 PM · Restricted Project, Restricted Project
void updated the diff for D69868: Allow "callbr" to return non-void values.

Use the BB when creating the MBB.

Feb 15 2020, 2:10 PM · Restricted Project, Restricted Project

Feb 14 2020

void added a comment to D69868: Allow "callbr" to return non-void values.

Another friendly ping. :-)

Feb 14 2020, 4:03 PM · Restricted Project, Restricted Project

Feb 10 2020

void committed rGc55cf4afa916: Revert "Remove redundant "std::move"s in return statements" (authored by void).
Revert "Remove redundant "std::move"s in return statements"
Feb 10 2020, 7:12 AM
void added a reverting change for rG1c2241a7936b: Remove redundant "std::move"s in return statements: rGc55cf4afa916: Revert "Remove redundant "std::move"s in return statements".
Feb 10 2020, 7:12 AM