Page MenuHomePhabricator

reames (Philip Reames)
User

Projects

User does not belong to any projects.

User Details

User Since
Oct 23 2013, 6:32 PM (377 w, 3 d)

Recent Activity

Yesterday

reames added a comment to D94542: [X86] Default to -x86-pad-for-align=false to drop assembler difference with or w/o -g.

The value of -mbranches-within-32B-boundaries also dilutes over time (it mitigates issues for some Skylake architectures).
(Surprising to me, this feature has been available for one year but I don't see a lot of adoption).

The performance impact of the microcode fix were highly variable depending on the exact details of each workload. Mostly barely moved, some really suffered. I know of a couple of organizations using the mitigation, but I agree with your general point about this being something that decays in value with time. Hopefully, in a few years, we can stop talking about this.

Fri, Jan 15, 8:37 AM · Restricted Project

Thu, Jan 14

reames added a comment to D94542: [X86] Default to -x86-pad-for-align=false to drop assembler difference with or w/o -g.

... is pretty important that turning on debug info does not change the generated machine code - otherwise there's the chance of heisenbugs.

If this is a case where enabling debug info changes the selected machine code, that is a bug and one worth fixing one way or another.

David, thanks for chiming in here. Reading your wording made me realize I was wrong in my take here. I had been thinking of this as a debug info quality problem, but you're right, that's not what is actually going on here.

Thu, Jan 14, 9:39 PM · Restricted Project
reames added a comment to D94542: [X86] Default to -x86-pad-for-align=false to drop assembler difference with or w/o -g.

I'm not convinced that disabling these optimizations is warranted. I'm not actively opposing the patch, just want to list my concerns for the record.

Thu, Jan 14, 7:58 PM · Restricted Project
reames accepted D94703: [Statepoint] Handle `undef` operands in statepoint..

LGTM w/an MIR test case added before submission.

Thu, Jan 14, 7:41 PM · Restricted Project

Tue, Jan 12

reames added a comment to D94542: [X86] Default to -x86-pad-for-align=false to drop assembler difference with or w/o -g.

I also ask that a bit more background be given to justify this change. I found the bug (https://bugs.llvm.org/show_bug.cgi?id=42138#c13), but that gives no information about the cause of the assembly difference. Has anyone examined the cause of the labels being emitted in debug mode to see if they're necessary/useful?

I explained the cause in the description: "A -g build has more MCSymbol's and therefore may have different assembler output (e.g. a MCRelaxableFragment (jmp) may have 5 bytes with -O1 while 2 bytes with -O1 -g)."

This doesn't get at the root cause though. Are those labels doing anything in the debug build? Why are they emitted? Can they be reasonably removed?

Tue, Jan 12, 6:13 PM · Restricted Project
reames added a comment to D75203: [X86] Relax existing instructions to reduce the number of nops needed for alignment purposes.

This review has been closed for nearly 10 months. Please move discussion of proposed changes to the bug or another relevant location.

Tue, Jan 12, 4:56 PM · Restricted Project
reames added a comment to D94542: [X86] Default to -x86-pad-for-align=false to drop assembler difference with or w/o -g.

Both the align and branch handling are "optimizations". I object to one being enabled and the other disabled. If you want them both on by default, fine. If you want them both off by default, fine. Having one off and one on is confusing.

Tue, Jan 12, 4:55 PM · Restricted Project
reames added a comment to rG4b33b2387787: Reapply "[LV] Vectorize (some) early and multiple exit loops"" w/fix for builder.

This commit seem to cause

$ opt -verify -loop-vectorize bbi-51639.ll -S
opt: /repo/elavkje/llvm-project-upstream/llvm/lib/Transforms/Utils/LoopVersioning.cpp:47: llvm::LoopVersioning::LoopVersioning(const llvm::LoopAccessInfo &, ArrayRef<llvm::RuntimePointerCheck>, llvm::Loop *, llvm::LoopInfo *, llvm::DominatorTree *, llv
m::ScalarEvolution *): Assertion `L->getExitBlock() && "No single exit block"' failed.

Fixed in commit caafdf07b. This was a spurious assert.

Tue, Jan 12, 12:58 PM
reames committed rGcaafdf07bbcc: [LV] Weaken spuriously strong assert in LoopVersioning (authored by reames).
[LV] Weaken spuriously strong assert in LoopVersioning
Tue, Jan 12, 12:57 PM
reames updated the diff for D93865: [LV] Vectorize (some) early and multiple exit loops w/tail folding.

rebase and ping

Tue, Jan 12, 12:47 PM · Restricted Project
reames committed rG9f61fbd75ae1: [LV] Relax assumption that LCSSA implies single entry (authored by reames).
[LV] Relax assumption that LCSSA implies single entry
Tue, Jan 12, 12:35 PM
reames closed D93725: [LV] Relax assumption that LCSSA implies single entry.
Tue, Jan 12, 12:35 PM · Restricted Project
reames accepted D94483: [Verifier] Add tied-ness verification to statepoint intsruction.

LGTM w/changes.

Tue, Jan 12, 9:07 AM · Restricted Project
reames added a comment to D94482: [Verifier] Extend statepoint verifier to cover more constants.

Side note: The -1/+1s appearing everywhere confused me until I looked into the getConstMetaVal function. Having the index returned be the second in the tag, value pair seems slightly confusing, but it fits the existing structure.

Tue, Jan 12, 9:03 AM · Restricted Project
reames added a comment to D94469: [Statepoint Lowering] Add an option to allow use gc values in regs for landing pad.

LGTM as a testing aid. Should be removed once invoke support is stabilized and fully enabled.

Tue, Jan 12, 8:54 AM · Restricted Project
reames added a comment to D94389: [InlineSpiller]Re-tie operands if folding failed.

Also LGTM. Thanks.

Tue, Jan 12, 8:52 AM · Restricted Project

Mon, Jan 11

reames added a comment to D94389: [InlineSpiller]Re-tie operands if folding failed.

I would strongly prefer that instead of simply asserting that we retie the registers. Doing so should be fairly simple - we just need to keep track of which register pairs we untied for the possible fold, and use a lambda to retie them.

Mon, Jan 11, 9:26 AM · Restricted Project

Sun, Jan 10

reames added a comment to D94248: [NFC][FuncAttrs] Add comment on why we look for noreturn calls.

I'm dropping my request which triggered this review. Feel free to close.

Sun, Jan 10, 5:32 PM · Restricted Project
reames requested review of D94378: [LoopDeletion] Handle inner loops w/untaken backedges.
Sun, Jan 10, 5:08 PM · Restricted Project
reames committed rG4739dd67e7a0: [LoopDeletion] Break backedge of outermost loops when known not taken (authored by reames).
[LoopDeletion] Break backedge of outermost loops when known not taken
Sun, Jan 10, 4:03 PM
reames closed D93906: [LoopDeletion] Break backedge of loops when known not taken.
Sun, Jan 10, 4:02 PM · Restricted Project
reames planned changes to D93906: [LoopDeletion] Break backedge of loops when known not taken.
Sun, Jan 10, 12:56 PM · Restricted Project
reames reopened D93906: [LoopDeletion] Break backedge of loops when known not taken.
Sun, Jan 10, 12:56 PM · Restricted Project
reames updated the diff for D93906: [LoopDeletion] Break backedge of loops when known not taken.

Context: This patch was reverted following failures in stage2 builders (only). Still investigating root cause. Reproduction is a bit of a pain.

Sun, Jan 10, 12:56 PM · Restricted Project
reames committed rGfc8ab2544729: [Tests] Precommit tests from to simplify rebase (authored by reames).
[Tests] Precommit tests from to simplify rebase
Sun, Jan 10, 12:42 PM
reames updated the diff for D93725: [LV] Relax assumption that LCSSA implies single entry.

rebase over landed tests

Sun, Jan 10, 12:37 PM · Restricted Project
reames committed rG86d6f7e90a1d: Precommit tests requested for D93725 (authored by reames).
Precommit tests requested for D93725
Sun, Jan 10, 12:30 PM
reames committed rG377dcfd5c15d: [Tests] Auto update a vectorizer test to simplify future diff (authored by reames).
[Tests] Auto update a vectorizer test to simplify future diff
Sun, Jan 10, 12:24 PM
reames updated the diff for D93725: [LV] Relax assumption that LCSSA implies single entry.

Add requested test.

Sun, Jan 10, 12:18 PM · Restricted Project
reames updated the diff for D93725: [LV] Relax assumption that LCSSA implies single entry.

Address stylistic updates. Test changes to be done separately in near future.

Sun, Jan 10, 11:48 AM · Restricted Project

Mon, Jan 4

reames added a reverting change for rGdd6bb367d19e: [LoopDeletion] Break backedge of loops when known not taken: rG7c63aac7bd4e: Revert "[LoopDeletion] Break backedge of loops when known not taken".
Mon, Jan 4, 9:52 AM
reames committed rG7c63aac7bd4e: Revert "[LoopDeletion] Break backedge of loops when known not taken" (authored by reames).
Revert "[LoopDeletion] Break backedge of loops when known not taken"
Mon, Jan 4, 9:52 AM
reames added a reverting change for D93906: [LoopDeletion] Break backedge of loops when known not taken: rG7c63aac7bd4e: Revert "[LoopDeletion] Break backedge of loops when known not taken".
Mon, Jan 4, 9:52 AM · Restricted Project
reames added a comment to D93906: [LoopDeletion] Break backedge of loops when known not taken.

Isn't this already covered by the zero exit count optimization in IndVars?

No. I believe you're referring to optimizeLoopExits. That does not modify the CFG, it just folds conditions to constants. This will specifically modify the CFG and remove the loop.

Yes, that's the transform I had in mind. I see the difference now, but I'm not sure I understand the larger picture / motivation behind this. The true/false branches will get folded away by SimplifyCFG -- so is this a phase ordering problem where SimplifyCFG runs too late (if so, can we add a PhaseOrdering test)? Should we also try to fold away (to unreachable) dead exits, rather than just dead backedges?

You can view this as a phase ordering issue, but really I see it as "this pass is supposed to delete dead loops" + "a case came up in discussion which we'd missed" + "easy no-cost fix".

Mon, Jan 4, 9:37 AM · Restricted Project
reames committed rGdd6bb367d19e: [LoopDeletion] Break backedge of loops when known not taken (authored by reames).
[LoopDeletion] Break backedge of loops when known not taken
Mon, Jan 4, 9:25 AM
reames closed D93906: [LoopDeletion] Break backedge of loops when known not taken.
Mon, Jan 4, 9:25 AM · Restricted Project

Sun, Jan 3

reames added a comment to D93906: [LoopDeletion] Break backedge of loops when known not taken.

Isn't this already covered by the zero exit count optimization in IndVars?

No. I believe you're referring to optimizeLoopExits. That does not modify the CFG, it just folds conditions to constants. This will specifically modify the CFG and remove the loop.

Sun, Jan 3, 2:47 PM · Restricted Project
reames added a comment to D93906: [LoopDeletion] Break backedge of loops when known not taken.

ping

Sun, Jan 3, 11:34 AM · Restricted Project
reames added a comment to D93725: [LV] Relax assumption that LCSSA implies single entry.

ping

Sun, Jan 3, 11:34 AM · Restricted Project

Tue, Dec 29

reames requested review of D93906: [LoopDeletion] Break backedge of loops when known not taken.
Tue, Dec 29, 10:41 AM · Restricted Project

Mon, Dec 28

reames added a comment to D93764: [LoopUnswitch] Implement first version of partial unswitching..

I think this is a separate issue (the condition is dependent on the induction variable), but nevertheless interesting to pursue. IIRC there is already a pass that tries to do something similar, but I don't know which one of the top of my head. Will check.

This is InductiveRangeCheckElimination which implements a form of iteration set splitting. I'll warn that IRCE has been historically bug prone, and is not on by default upstream.

Mon, Dec 28, 8:42 PM · Restricted Project
reames requested review of D93865: [LV] Vectorize (some) early and multiple exit loops w/tail folding.
Mon, Dec 28, 10:36 AM · Restricted Project
reames added a reverting change for rG4ffcd4fe9ac2: Revert "[LV] Vectorize (some) early and multiple exit loops": rG4b33b2387787: Reapply "[LV] Vectorize (some) early and multiple exit loops"" w/fix for builder.
Mon, Dec 28, 10:15 AM
reames committed rG4b33b2387787: Reapply "[LV] Vectorize (some) early and multiple exit loops"" w/fix for builder (authored by reames).
Reapply "[LV] Vectorize (some) early and multiple exit loops"" w/fix for builder
Mon, Dec 28, 10:15 AM
reames added a comment to D93317: [LV] Vectorize (some) early and multiple exit loops.

Ayal, Florian, thank you for the efforts on this review. I really appreciate the active review.

Mon, Dec 28, 9:45 AM · Restricted Project
reames added a comment to D93725: [LV] Relax assumption that LCSSA implies single entry.

ping now that previous patch in series has landed

Mon, Dec 28, 9:41 AM · Restricted Project
reames committed rGe4df6a40dad6: [LV] Vectorize (some) early and multiple exit loops (authored by reames).
[LV] Vectorize (some) early and multiple exit loops
Mon, Dec 28, 9:41 AM
reames closed D93317: [LV] Vectorize (some) early and multiple exit loops.
Mon, Dec 28, 9:41 AM · Restricted Project

Sat, Dec 26

reames accepted D93810: [RS4GC] Lazily set changed flag when folding single entry phis.

LGTM

Sat, Dec 26, 11:14 AM · Restricted Project

Wed, Dec 23

reames added a comment to D93317: [LV] Vectorize (some) early and multiple exit loops.

Florian, any other edge cases you can think of? I'd completely missed that one. Thank you for finding it!

I think there are some scenarios when we break some LCSSA PHIs that use PHIs created during SCEV expansion when we generate the runtime checks. After a first glance, it appears like an issue after we add the conditional branch from the middle block. The example below should cause a verifier failure with opt -loop-vectorize -force-vector-width=4. I tried to reduce it a bit, but unfortunately it is still quite ugly.

I can confirm the failure and will debug. Thank you again for finding a cornercase.

Wed, Dec 23, 11:26 AM · Restricted Project

Tue, Dec 22

reames added a comment to D93734: [LoopDeletion] Insert an early exit from dead path in loop.

As implemented, this is a bit too specific to one particular pattern to be commitable, but I think you've got the seed of a good idea here.

Tue, Dec 22, 8:32 PM · Restricted Project
reames accepted D93716: [LoopDeletion] Also consider loops with subloops for deletion..
Tue, Dec 22, 12:37 PM · Restricted Project
reames added a comment to D93716: [LoopDeletion] Also consider loops with subloops for deletion..

Reading through the code, I had a couple of separate thoughts. If you're interested, feel free to follow up on these. If not, feel free to disregard.

Tue, Dec 22, 12:37 PM · Restricted Project
reames requested review of D93725: [LV] Relax assumption that LCSSA implies single entry.
Tue, Dec 22, 12:23 PM · Restricted Project
reames updated the diff for D93317: [LV] Vectorize (some) early and multiple exit loops.

Incorporate a fix for the issue Florian found. Essentially, we can't both treat single use exit conditions as dead, and allow predication to use them in a mask. Since we know they're evaluate to true for the entire vector body, we can simply ignore them when forming edge predicates.

Tue, Dec 22, 11:49 AM · Restricted Project
reames added a comment to D93317: [LV] Vectorize (some) early and multiple exit loops.

Ok, I see what's going on w/Florian's example. It's an interaction with block predication and early exits. Essentially, block predication expects to be able to add uses of any condition in the loop, and the dead code elimination has eliminated the exit condition in the vector loop. I can restrict the legality slightly to avoid this interaction easily enough, but I want to think a bit first if there's a clean integration.

Tue, Dec 22, 10:09 AM · Restricted Project
reames added a comment to D93317: [LV] Vectorize (some) early and multiple exit loops.

JFYI, multiple latch tests added in f106b2, but they don't impact the diff as SCEV can't prove exit counts and thus we don't vectorize.

Tue, Dec 22, 10:00 AM · Restricted Project
reames committed rGf106b281be24: [tests] precommit a test mentioned in review for D93317 (authored by reames).
[tests] precommit a test mentioned in review for D93317
Tue, Dec 22, 9:49 AM
reames updated the diff for D93317: [LV] Vectorize (some) early and multiple exit loops.

Incorporate wording/style comments from Ayal, test rebase pending.

Tue, Dec 22, 9:35 AM · Restricted Project
reames added a comment to D93317: [LV] Vectorize (some) early and multiple exit loops.

Ayal, thanks for all the great wordsmith comments!

Tue, Dec 22, 9:24 AM · Restricted Project

Fri, Dec 18

reames added a comment to D93317: [LV] Vectorize (some) early and multiple exit loops.

ping

Fri, Dec 18, 10:18 AM · Restricted Project

Dec 15 2020

reames committed rG1f6e15566f14: [LV] Weaken a unnecessarily strong assert [NFC] (authored by reames).
[LV] Weaken a unnecessarily strong assert [NFC]
Dec 15 2020, 7:08 PM
reames committed rGaf7ef895d495: [LV] Extend dead instruction detection to multiple exiting blocks (authored by reames).
[LV] Extend dead instruction detection to multiple exiting blocks
Dec 15 2020, 6:50 PM
reames updated the diff for D93317: [LV] Vectorize (some) early and multiple exit loops.

One further simplification enabled by previous rebase.

Dec 15 2020, 12:55 PM · Restricted Project
reames updated the diff for D93317: [LV] Vectorize (some) early and multiple exit loops.

Rebase over a81db8b31. While that change is NFC, the code structure makes it much easier to disable vectorization when predication is demanded and we can't provide it.

Dec 15 2020, 12:45 PM · Restricted Project
reames committed rGa81db8b3159e: [LV] Restructure handling of -prefer-predicate-over-epilogue option [NFC] (authored by reames).
[LV] Restructure handling of -prefer-predicate-over-epilogue option [NFC]
Dec 15 2020, 12:38 PM
reames updated the diff for D93317: [LV] Vectorize (some) early and multiple exit loops.

Update tests to cover cases where we can't vectorize due to either a) size, or b) predication.

Dec 15 2020, 12:06 PM · Restricted Project
reames committed rGa048e2fa1d02: [tests] fix an accidental target dependence added in 99ac8868 (authored by reames).
[tests] fix an accidental target dependence added in 99ac8868
Dec 15 2020, 11:08 AM
reames updated the summary of D93317: [LV] Vectorize (some) early and multiple exit loops.
Dec 15 2020, 11:02 AM · Restricted Project
reames updated the diff for D93317: [LV] Vectorize (some) early and multiple exit loops.

rebase over landed tests

Dec 15 2020, 11:01 AM · Restricted Project
reames committed rG99ac8868cfb4: [tests][LV] precommit tests for D93317 (authored by reames).
[tests][LV] precommit tests for D93317
Dec 15 2020, 10:55 AM
reames added a reviewer for D93317: [LV] Vectorize (some) early and multiple exit loops: anna.
Dec 15 2020, 10:39 AM · Restricted Project
reames requested review of D93317: [LV] Vectorize (some) early and multiple exit loops.
Dec 15 2020, 10:38 AM · Restricted Project

Dec 14 2020

reames committed rG3b3eb7f07ff9: Speculative fix for build bot failures (authored by reames).
Speculative fix for build bot failures
Dec 14 2020, 1:45 PM
reames committed rGf5fe8493e5ac: [LAA] Relax restrictions on early exits in loop structure (authored by reames).
[LAA] Relax restrictions on early exits in loop structure
Dec 14 2020, 12:44 PM
reames closed D92066: [LAA] Relax restrictions on early exits in loop structure.
Dec 14 2020, 12:44 PM · Restricted Project
reames added inline comments to D92066: [LAA] Relax restrictions on early exits in loop structure.
Dec 14 2020, 12:29 PM · Restricted Project

Dec 12 2020

reames accepted D93162: [BasicAA] Handle known non-zero variable index.

LGTM.

Dec 12 2020, 1:45 PM · Restricted Project

Dec 8 2020

reames committed rG5171b7b40e98: [indvars] Common a bit of code [NFC] (authored by reames).
[indvars] Common a bit of code [NFC]
Dec 8 2020, 3:26 PM

Dec 7 2020

reames committed rG2656885390f1: Teach isKnownNonEqual how to recurse through invertible multiplies (authored by reames).
Teach isKnownNonEqual how to recurse through invertible multiplies
Dec 7 2020, 2:52 PM
reames closed D92726: Teach isKnownNonEqual how to recurse through invertable multiplies.
Dec 7 2020, 2:52 PM · Restricted Project
reames updated the diff for D92726: Teach isKnownNonEqual how to recurse through invertable multiplies.

Address review comments.

Dec 7 2020, 2:17 PM · Restricted Project
reames accepted D50010: [VNCoercion] Disallow coercion between different ni addrspaces.

As a side note, I don't think you actually need to disallow conversion of the null pointer between two different NI address spaces. If you wanted to improve opt quality, folding your check into the previous one would be beneficial. I'm not going to require that as this a) fixes a correctness bug, and b) has been outstanding for way to long.

Dec 7 2020, 11:24 AM · Restricted Project, Restricted Project
reames requested changes to D92488: [LICM] Add a maximum for the number of instructions to be hoisted.

Hard reject. This patch will not be accepted. This is a major design change.

Dec 7 2020, 11:21 AM · Restricted Project
reames updated the diff for D92726: Teach isKnownNonEqual how to recurse through invertable multiplies.

Add non-zero restriction. For now, restrict to constant case. I could have used isKnownNonZero, but that would require recursing through both operands which I'd rather not do from a compile time perspective.

Dec 7 2020, 11:15 AM · Restricted Project

Dec 5 2020

reames requested review of D92726: Teach isKnownNonEqual how to recurse through invertable multiplies.
Dec 5 2020, 5:18 PM · Restricted Project
reames accepted D92723: [BasicAA] Migrate "same base pointer" logic to decomposed GEPs.

LGTM. I had to grab a sheet of paper and run some examples to convince myself that your code worked - even with the explanation - but I got there eventually. :)

Dec 5 2020, 4:33 PM · Restricted Project
reames committed rG8f076291be41: Add recursive decomposition reasoning to isKnownNonEqual (authored by reames).
Add recursive decomposition reasoning to isKnownNonEqual
Dec 5 2020, 3:58 PM
reames closed D92698: Add recursive decomposition reasoning to isKnownNonEqual.
Dec 5 2020, 3:58 PM · Restricted Project
reames added inline comments to D92698: Add recursive decomposition reasoning to isKnownNonEqual.
Dec 5 2020, 3:56 PM · Restricted Project
reames requested changes to D92723: [BasicAA] Migrate "same base pointer" logic to decomposed GEPs.

Ok, you and I are clearly thinking about the same problems. :) I have a patch which I hadn't yet posted to handle this exact same case. I just needed to get prerequisites out of the way in terms of D92698 and D92694. I'm happy to defer to you and act as knowledgeable reviewer though!

Dec 5 2020, 2:48 PM · Restricted Project
reames resigned from D91460: [AsmParser] make .ascii support spaces as separators.
Dec 5 2020, 2:36 PM · Restricted Project
reames updated the diff for D92698: Add recursive decomposition reasoning to isKnownNonEqual.

Remove ptrtoint and inttoptr due to truncation issue pointed out in review. These can be handled in a follow up patch if needed, my motivating case doesn't require them.

Dec 5 2020, 2:24 PM · Restricted Project
reames added a comment to D92698: Add recursive decomposition reasoning to isKnownNonEqual.

Could you add some tests for the BasicAA use case?

Tests for the basic aa extension will be in the patch for basicaa. I could probably contrive a test case through the existing basic aa use of isKnownNonEqual, but it would take some thought. Are you okay with me landing this as is, and then including supplemental tests in the next patch?

Dec 5 2020, 2:22 PM · Restricted Project
reames committed rGbfda69416c6d: [BasicAA] Fix a bug with relational reasoning across iterations (authored by reames).
[BasicAA] Fix a bug with relational reasoning across iterations
Dec 5 2020, 2:10 PM
reames closed D92694: [BasicAA] Fix a bug with relational reasoning across iterations.
Dec 5 2020, 2:10 PM · Restricted Project

Dec 4 2020

reames updated the diff for D92698: Add recursive decomposition reasoning to isKnownNonEqual.

Rebase over landed tests

Dec 4 2020, 3:21 PM · Restricted Project
reames committed rG99f79cbf31cc: [test] precommit test for D92698 (authored by reames).
[test] precommit test for D92698
Dec 4 2020, 3:19 PM
reames requested review of D92698: Add recursive decomposition reasoning to isKnownNonEqual.
Dec 4 2020, 3:10 PM · Restricted Project
reames added inline comments to D92694: [BasicAA] Fix a bug with relational reasoning across iterations.
Dec 4 2020, 2:24 PM · Restricted Project