Page MenuHomePhabricator

reames (Philip Reames)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Thu, Sep 24

reames requested changes to D88208: [SCEV] Prove implicaitons via AddRec start.
Thu, Sep 24, 9:57 AM · Restricted Project
reames added a comment to D87148: [ImplicitNullCheck] Handle Nonzero faulting pages and complex addressing.

Looks close to ready. Please address review comments, and we'll probably be ready for an LGTM.

Thu, Sep 24, 9:35 AM · Restricted Project

Tue, Sep 22

reames committed rGe1a3271ebb87: [AArch64] Teach analyzeBranch to remove branch equivelent to fallthrough (authored by reames).
[AArch64] Teach analyzeBranch to remove branch equivelent to fallthrough
Tue, Sep 22, 2:38 PM
reames closed D88035: [AArch64] Teach analyzeBranch to remove branch equivelent to fallthrough.
Tue, Sep 22, 2:38 PM · Restricted Project
reames updated the diff for D88035: [AArch64] Teach analyzeBranch to remove branch equivelent to fallthrough.

address review comment

Tue, Sep 22, 8:22 AM · Restricted Project
reames added inline comments to D88035: [AArch64] Teach analyzeBranch to remove branch equivelent to fallthrough.
Tue, Sep 22, 8:07 AM · Restricted Project

Mon, Sep 21

reames requested review of D88035: [AArch64] Teach analyzeBranch to remove branch equivelent to fallthrough.
Mon, Sep 21, 10:21 AM · Restricted Project

Fri, Sep 18

reames committed rG06f136f61e6d: [instcombine][x86] Converted pdep/pext with shifted mask to simple arithmetic (authored by reames).
[instcombine][x86] Converted pdep/pext with shifted mask to simple arithmetic
Fri, Sep 18, 2:55 PM
reames closed D87861: [instcombine][x86] Converted pdep/pext with shifted mask to simple arithmetic.
Fri, Sep 18, 2:55 PM · Restricted Project

Thu, Sep 17

reames committed rGb4013f9c7feb: [MemorySSA] Fix an unused variable warning [NFC] (authored by reames).
[MemorySSA] Fix an unused variable warning [NFC]
Thu, Sep 17, 4:08 PM
reames committed rGb04c181ed776: [AArch64] Enable implicit null check transformation (authored by reames).
[AArch64] Enable implicit null check transformation
Thu, Sep 17, 4:02 PM
reames closed D87851: [AArch64] Enable implicit null check transformation.
Thu, Sep 17, 4:02 PM · Restricted Project
reames requested review of D87861: [instcombine][x86] Converted pdep/pext with shifted mask to simple arithmetic.
Thu, Sep 17, 3:20 PM · Restricted Project
reames accepted D67178: [SCEV] Use loop guard info when computing the max BE taken count in howFarToZero..

LGTM. This makes a nice starting point - as you note, we can definitely extend this.

Thu, Sep 17, 1:55 PM · Restricted Project
reames requested review of D87851: [AArch64] Enable implicit null check transformation.
Thu, Sep 17, 12:35 PM · Restricted Project

Wed, Sep 16

reames committed rG7af4f44c3e3d: [aarch64][tests] Add tests which show current lack of implicit null support (authored by reames).
[aarch64][tests] Add tests which show current lack of implicit null support
Wed, Sep 16, 12:55 PM

Tue, Sep 15

reames accepted D87224: [InstCombine] Add tests for statepoint simplification.

LGTM - if you're just adding tests, you don't need separate review as long as there's nothing fancy.

Tue, Sep 15, 11:56 AM · Restricted Project
reames added inline comments to D66688: [LoopVectorize] Leverage speculation safety to avoid masked.loads.
Tue, Sep 15, 11:55 AM · Restricted Project
reames added inline comments to D66688: [LoopVectorize] Leverage speculation safety to avoid masked.loads.
Tue, Sep 15, 11:53 AM · Restricted Project
reames added a comment to D87344: [IndVars] Remove exiting conditions that are trivially true/false.

Max, I think this is probably handled by the existing optimizeLoopExits if the exit counts for the condition are computable. (i.e. you're probably looking at a gap in exit count computation)

Tue, Sep 15, 11:39 AM · Restricted Project
reames added inline comments to D66688: [LoopVectorize] Leverage speculation safety to avoid masked.loads.
Tue, Sep 15, 11:36 AM · Restricted Project
reames accepted D87695: [Statepoints][ISEL] gc.relocate uniquification should be based on SDValue, not IR Value..

However, I think this a workaround (and a useful optimization) not a true fix. What would happen if for some reason a COPY didn't get eliminated? (Imagine no duplicates, just a single pointer being relocated, with relocated value used down the exceptional path) I don't have a test case for this, but I'd advise digging into that possibility further. I'd need to study invoke lowering more closely to understand the risk.

Tue, Sep 15, 9:34 AM · Restricted Project

Mon, Sep 14

reames added a comment to D66012: [AArch64][Statepoints] Statepoint support for AArch64..

When rebasing, I discovered the tests for this were from before the operand bundle format change. Given that, I ended up dropping most of the tests. I'll land a couple more in a follow up, but overall, it seems reasonable to have most testing in the x86 target. We don't need to exercise all cornercases in both places.

Mon, Sep 14, 4:44 PM · Restricted Project
reames committed rGe6bc7037d386: [AArch64] Statepoint support for AArch64. (authored by reames).
[AArch64] Statepoint support for AArch64.
Mon, Sep 14, 4:43 PM
reames closed D66012: [AArch64][Statepoints] Statepoint support for AArch64..
Mon, Sep 14, 4:43 PM · Restricted Project
reames accepted D66012: [AArch64][Statepoints] Statepoint support for AArch64..

LGTM as well.

Mon, Sep 14, 2:28 PM · Restricted Project
reames accepted D87480: [InstCombine] Simplify select operand based on equality condition.

LGTM

Mon, Sep 14, 8:26 AM · Restricted Project

Fri, Sep 11

reames requested changes to D87148: [ImplicitNullCheck] Handle Nonzero faulting pages and complex addressing.

Must fix:

  • Your change does not include a LangRef update for the new metadata.
  • The name chosen for the metadata is ambiguous. I'd also suggest phrasing it in terms of ranges, not page starts. Maybe: guaranteed_faulting_ranges, or implicit_check_faulting_ranges?
  • As you note in the review, this is mixing two sets of changes in a way which makes it hard to reason about either in isolation. Please split. Either order is fine as you should be able to test either piece in isolation without the other.
Fri, Sep 11, 9:32 AM · Restricted Project

Tue, Sep 8

reames accepted D87108: [ImplicitNullCheck] Handle instructions that do not modify null behaviour of null checked reg.

LGTM

Tue, Sep 8, 2:10 PM · Restricted Project
reames accepted D87251: [Statepoints] Update DAG root after emitting statepoint..

LGTM

Tue, Sep 8, 2:05 PM · Restricted Project
reames accepted D87252: [Statepoints] Properly handle const base pointer..

LGTM. Good catch, and nicely structured cleanup in the process.

Tue, Sep 8, 2:04 PM · Restricted Project

Fri, Sep 4

reames added a comment to D87108: [ImplicitNullCheck] Handle instructions that do not modify null behaviour of null checked reg.

Approach generally looks reasonable. Please address nitpicks and then I plan to LGTM.

Fri, Sep 4, 12:25 PM · Restricted Project

Aug 27 2020

reames accepted D86712: [Statepoint] Always spill base pointer..

LGTM

Aug 27 2020, 5:30 PM · Restricted Project

Aug 21 2020

reames accepted D86083: [SCEV] Add operand methods to Exprs.

LGTM

Aug 21 2020, 8:30 AM · Restricted Project
reames accepted D85954: [InstCombine] Move handling of gc.relocate in a gc.statepoint.
Aug 21 2020, 8:25 AM · Restricted Project
reames added a comment to D85954: [InstCombine] Move handling of gc.relocate in a gc.statepoint.

As a follow up, it would be nice to handle two additional optimizations:

  1. Canonicalize each pointer to a single index in the statepoint operand list
  2. Shrink the gc-live bundle to remove any pointers not needed after (1)
Aug 21 2020, 8:25 AM · Restricted Project

Aug 14 2020

reames committed rG1621c004da7b: [Tests] Be consistent w/definition of statepoint-example (authored by reames).
[Tests] Be consistent w/definition of statepoint-example
Aug 14 2020, 8:48 PM
reames committed rG6b2105456a1a: [Statepoint] Remove code related to inline operand bundles (authored by reames).
[Statepoint] Remove code related to inline operand bundles
Aug 14 2020, 8:32 PM
reames committed rG48f4312d4ec7: Remove inline gc arguments from statepoints (authored by reames).
Remove inline gc arguments from statepoints
Aug 14 2020, 7:45 PM
reames committed rGa96fc4638b73: Remove deopt and gc transition arguments from gc.statepoint intrinsic (authored by reames).
Remove deopt and gc transition arguments from gc.statepoint intrinsic
Aug 14 2020, 4:08 PM
reames closed D80892: Remove deopt and gc transition arguments from gc.statepoint intrinsic.
Aug 14 2020, 4:08 PM · Restricted Project
reames abandoned D84964: [WIP] Demo a functional problem from D81647 with a fix and test case.
Aug 14 2020, 3:54 PM · Restricted Project

Aug 13 2020

reames accepted D75598: [InstCombine] Handle gc.relocate(null) in one iteration.

Serguei and I discussed this one fairly extensively offline. Long term, we're probably going to move to a model where we process all of the projections for a statepoint in a single pass when visiting the statepoint, but for the moment, we need to decrease the number of instcombine iterations. In particular, with recent efforts to clamp the number of iterations in debug builds, we are seeing R+A crashes in downstream tests, so this change is really a regression fix.

Aug 13 2020, 7:44 AM · Restricted Project

Jul 30 2020

reames accepted D81646: MIR Statepoint refactoring. Part 2: Operand folding..
Jul 30 2020, 7:16 PM · Restricted Project
reames added a comment to D81646: MIR Statepoint refactoring. Part 2: Operand folding..

LGTM. I sat down today to write a cleaner patch, and in the process convinced myself this was in fact correct. I will post a separate cleanup once this lands as I think we can have the foldMemoryOperand impl structured much more cleanly for tied operands, but doing so involves a slightly deeper rewrite of some x86 specific code than is reasonable to ask for here.

Jul 30 2020, 7:16 PM · Restricted Project
reames added a comment to D81648: MIR Statepoint refactoring. Part 4: ISEL changes..

Second, there appears to be a semantic problem around the handling of base vs derived slots unless we *always* spill the base. We can't tie both uses to a single def. This may warrant some offline discussion.

Yes, we can't. But apparently, we do not need to.
Basically, that tied-ness information is only needed to glue two live ranges into one during SSA flattening. And it does not matter how many uses end live range at the same instruction.
Luckily, LLVM appears to handle it properly.

But if included tests (and checks) do not make you believe it works, we should throw all this stuff away and try some different approach.

I filed a bug with a detailed description of the problem and one suggestion as to how to approach. See https://bugs.llvm.org/show_bug.cgi?id=46917, let's take discussion there (or offline).

Jul 30 2020, 12:04 PM · Restricted Project
reames added inline comments to D81647: MIR Statepoint refactoring. Part 3: Spill GC Ptr regs..
Jul 30 2020, 11:30 AM · Restricted Project
reames requested review of D84964: [WIP] Demo a functional problem from D81647 with a fix and test case.
Jul 30 2020, 11:29 AM · Restricted Project

Jul 29 2020

reames committed rG755f91f12cf0: [Statepoint] Enable cross block relocates w/vreg lowering (authored by reames).
[Statepoint] Enable cross block relocates w/vreg lowering
Jul 29 2020, 1:33 PM
reames committed rGe980913831c1: [Tests] Split a file for ease of update (authored by reames).
[Tests] Split a file for ease of update
Jul 29 2020, 12:45 PM
reames committed rG8fe2abc190f9: [Statepoint] Consolidate relocation type tracking [NFC] (authored by reames).
[Statepoint] Consolidate relocation type tracking [NFC]
Jul 29 2020, 11:46 AM
reames requested changes to D83965: MIR Statepoint refactoring. Part 5: Handle non-local relocates in ISEL..

JFYI, I've posted (and now landed after review) the subset of this patch which is mostly NFC here: https://reviews.llvm.org/D84692

Jul 29 2020, 9:26 AM · Restricted Project
reames committed rG31342eb63e93: [Statepoint] When using the tied def lowering, unconditionally use vregs… (authored by reames).
[Statepoint] When using the tied def lowering, unconditionally use vregs…
Jul 29 2020, 9:24 AM
reames closed D84692: [Statepoint] When using the tied def lowering, unconditionally use vregs [almost NFC].
Jul 29 2020, 9:24 AM · Restricted Project

Jul 27 2020

reames updated the diff for D84692: [Statepoint] When using the tied def lowering, unconditionally use vregs [almost NFC].

Address Register related review comments

Jul 27 2020, 12:34 PM · Restricted Project
Herald added a project to D84692: [Statepoint] When using the tied def lowering, unconditionally use vregs [almost NFC]: Restricted Project.
Jul 27 2020, 12:08 PM · Restricted Project

Jul 25 2020

reames committed rG55dae9c20ce3: [Statepoints] Style cleanup after 3da1a963 [NFC] (authored by reames).
[Statepoints] Style cleanup after 3da1a963 [NFC]
Jul 25 2020, 4:41 PM
reames committed rG3da1a9634eb9: [Statepoints] Support lowering gc relocations to virtual registers (authored by reames).
[Statepoints] Support lowering gc relocations to virtual registers
Jul 25 2020, 2:26 PM
reames closed D81648: MIR Statepoint refactoring. Part 4: ISEL changes..
Jul 25 2020, 2:26 PM · Restricted Project
reames accepted D81648: MIR Statepoint refactoring. Part 4: ISEL changes..

I'm approving this patch not because it's perfect, but because it doesn't break anything in the existing code and iterating here is no longer productive. Once this lands, we can continue the discussion about base/derived handling with specific concrete examples.

Jul 25 2020, 2:25 PM · Restricted Project

Jul 11 2020

reames requested changes to D81648: MIR Statepoint refactoring. Part 4: ISEL changes..

I took some time today to apply this patch with the attention of addressing some of the unaddressed comments and landing it to unblock progress here. However, I hit two problems.

Jul 11 2020, 3:26 PM · Restricted Project

Jul 10 2020

reames requested changes to D81648: MIR Statepoint refactoring. Part 4: ISEL changes..

(Comments on code outside StatepointLowering. Minor +1 question)

Jul 10 2020, 11:38 AM · Restricted Project
reames added a comment to D81648: MIR Statepoint refactoring. Part 4: ISEL changes..

Detailed comments on the new implementation. These are on the whole minor. Remember to add your new test file.

Jul 10 2020, 11:26 AM · Restricted Project
reames added a comment to D81648: MIR Statepoint refactoring. Part 4: ISEL changes..

On first skim, looks much much better. I'm going to do a detailed pass through, but thank you for making the major design change requested.

Jul 10 2020, 11:07 AM · Restricted Project

Jul 9 2020

reames accepted D83440: [InstSimplify] Re-enable select ?, undef, X -> X transform when X is provably not poison.

And thank you for doing this.

Jul 9 2020, 11:29 AM · Restricted Project

Jul 7 2020

reames requested changes to D81648: MIR Statepoint refactoring. Part 4: ISEL changes..

I've spent several hours today wrapping my head around this patch. I think I've found a material simplification which should greatly simplify the code.

Jul 7 2020, 4:40 PM · Restricted Project
reames committed rG22596e7b2f3e: [Statepoint] Use early return to reduce nesting and clarify comments [NFC] (authored by reames).
[Statepoint] Use early return to reduce nesting and clarify comments [NFC]
Jul 7 2020, 4:19 PM
reames committed rG9955876d74a5: [Statepoint] Reduce intendation and change a variable name [NFC] (authored by reames).
[Statepoint] Reduce intendation and change a variable name [NFC]
Jul 7 2020, 4:19 PM
reames added a comment to D81648: MIR Statepoint refactoring. Part 4: ISEL changes..

Required Change - Please introduce a runtime flag which controls how many values are handled via vregs. Default this value to zero. This will remove all existing test diffs; if it doesn't you have a bug. Then introduce a new test file, optional copied from an existing one, called statepoint-gc-regs.ll which enumerates sufficient coverage for the new feature.

Jul 7 2020, 3:15 PM · Restricted Project
reames added a comment to D81648: MIR Statepoint refactoring. Part 4: ISEL changes..

Initial comments, continue to look for other suggestions.

Jul 7 2020, 2:14 PM · Restricted Project
reames committed rGb172cd781240: [Statepoint] Factor out logic for non-stack non-vreg lowering [almost NFC] (authored by reames).
[Statepoint] Factor out logic for non-stack non-vreg lowering [almost NFC]
Jul 7 2020, 1:35 PM

Jul 6 2020

reames requested changes to D82826: [X86] support .nops directive.
Jul 6 2020, 6:03 PM · Restricted Project
reames added a comment to D81166: [Matrix] Use nuw/nsw operand bundles for matrix.multiply..

(drive by comments only, please don't block on me)

Jul 6 2020, 5:13 PM · Restricted Project

Jun 26 2020

reames committed rG2e17bba32411: Migrate last batch of tests to gc-live bundle format (authored by reames).
Migrate last batch of tests to gc-live bundle format
Jun 26 2020, 10:56 AM

Jun 25 2020

reames committed rG5d65529e506c: Migrate a couple of codegen tests to gc-live format (authored by reames).
Migrate a couple of codegen tests to gc-live format
Jun 25 2020, 2:12 PM
reames committed rGb5769a777f1c: Migrate a couple of codegen tests to gc-live format (authored by reames).
Migrate a couple of codegen tests to gc-live format
Jun 25 2020, 1:06 PM

Jun 19 2020

reames accepted D82071: [IR] Convert profile metadata in createCallMatchingInvoke().

(Just for reference, the total bit tripped me up at first, but the reasoning is that we're converting to a call, not proving the call no throw. We could still throw out.)

Jun 19 2020, 9:39 PM · Restricted Project
reames requested changes to D81648: MIR Statepoint refactoring. Part 4: ISEL changes..

Denis, I'm having a really hard time wrapping my head around this code, even knowing what it is supposed to do. We need to clean this up a bit; it's not in a state where I can approve it.

Jun 19 2020, 3:14 PM · Restricted Project

Jun 15 2020

reames added a comment to D81765: Don't inline dynamic allocas that simplify to huge static allocas..

Just a thought, feel free to consider or ignore.

Jun 15 2020, 10:23 AM · Restricted Project
reames added a comment to D81646: MIR Statepoint refactoring. Part 2: Operand folding..

I can't wrap my head around why untieing operands would be correct here. It seems like we could end up with a use folded, but a def not leaving to invalid codegen? Can you either expand on the justification or discuss offline?

The key point here is that we have two-address form at this point (not SSA, i.e. register on both sides is the same: vreg1 = STATEPOINT vreg1<tied-def0>).
foldMemoryOperand is called from spillAroundUses in InlineSpiller. And this method (besides few other optimizations) for every VReg use tries to fold register into memory operand or, if unsuccessful, inserts reload before instruction.
In some sense it turns VReg into StackSlot. And since Def of stack slot is invisible to LLVM (just like with old statepoint), it "magically" works.

But you're converting an instruction which defines a vreg to one which doesn't. If the original definition vreg had uses, isn't this a miscompile?

Jun 15 2020, 10:23 AM · Restricted Project

Jun 12 2020

reames added a comment to D81646: MIR Statepoint refactoring. Part 2: Operand folding..

I can't wrap my head around why untieing operands would be correct here. It seems like we could end up with a use folded, but a def not leaving to invalid codegen? Can you either expand on the justification or discuss offline?

Jun 12 2020, 8:28 PM · Restricted Project

Jun 11 2020

reames accepted D81645: MIR Statepoint refactoring. Part 1: Basic MI level changes..
Jun 11 2020, 8:52 PM · Restricted Project
reames added a comment to D81645: MIR Statepoint refactoring. Part 1: Basic MI level changes..

LGTM with one minor stylistic question. Can be addressed in separate change if desired.

Jun 11 2020, 8:52 PM · Restricted Project

Jun 5 2020

reames committed rG32c09d527c26: [Tests] Migrate a number of tests to gc-live bundle representation (authored by reames).
[Tests] Migrate a number of tests to gc-live bundle representation
Jun 5 2020, 4:50 PM
reames requested changes to D81301: [X86] Emit two-byte NOP when possible.

Are two byte nops legal on all 32 bit x86? The comment seems to imply no.

Jun 5 2020, 2:36 PM · Restricted Project

Jun 4 2020

reames committed rG4c735439fd9a: [Statepoint] Migrate a few tests to gc-live bundle format and fix assert (authored by reames).
[Statepoint] Migrate a few tests to gc-live bundle format and fix assert
Jun 4 2020, 6:46 PM
reames committed rG3d40c7518985: [Statepoint] Switch RS4GC to using gc-live bundle form (authored by reames).
[Statepoint] Switch RS4GC to using gc-live bundle form
Jun 4 2020, 4:04 PM
reames closed D81121: [Statepoint] Switch RS4GC to using gc-live bundle form.
Jun 4 2020, 4:04 PM · Restricted Project
reames accepted D81181: [TargetLowering][NFC] More efficient emitPatchpoint()..

LGTM w/minor change suggested.

Jun 4 2020, 2:57 PM · Restricted Project
reames requested changes to D81181: [TargetLowering][NFC] More efficient emitPatchpoint()..

Agreed with Eli, simply having two passes over operands would be much cleaner.

Jun 4 2020, 12:42 PM · Restricted Project

Jun 3 2020

reames committed rGab6779bbd8f8: [Statepoint] Remove last of old ImmutableStatepoint code (authored by reames).
[Statepoint] Remove last of old ImmutableStatepoint code
Jun 3 2020, 8:54 PM
reames committed rG91dd2f253647: [Statepoint] Delete more dead code from old wrappers (authored by reames).
[Statepoint] Delete more dead code from old wrappers
Jun 3 2020, 8:22 PM
reames committed rGb9983c18fc3e: [Statepoint] Start the process of removing old interfaces (authored by reames).
[Statepoint] Start the process of removing old interfaces
Jun 3 2020, 8:22 PM
reames accepted D81095: [TableGen] Handle (outs variable_ops).

LGTM

Jun 3 2020, 7:17 PM · Restricted Project
reames committed rG382b3023cbbc: [Statepoints][CGP] Minor parameter type cleanup (authored by reames).
[Statepoints][CGP] Minor parameter type cleanup
Jun 3 2020, 4:03 PM
reames created D81121: [Statepoint] Switch RS4GC to using gc-live bundle form.
Jun 3 2020, 4:02 PM · Restricted Project
reames committed rGff529e0f2792: [Statepoint] Fix signed vs unsigned in index handling (authored by reames).
[Statepoint] Fix signed vs unsigned in index handling
Jun 3 2020, 3:30 PM
reames committed rG0e7c77053f56: Introduce a "gc-live" bundle for the gc arguments of a statepoint (authored by reames).
Introduce a "gc-live" bundle for the gc arguments of a statepoint
Jun 3 2020, 3:30 PM
reames closed D80937: Introduce a "gc-live" bundle for the gc arguments of a statepoint.

commit 0e7c77053f560d391fecbec5d5e0e42082865c8a

Jun 3 2020, 3:29 PM · Restricted Project
reames added inline comments to D80937: Introduce a "gc-live" bundle for the gc arguments of a statepoint.
Jun 3 2020, 3:29 PM · Restricted Project