Page MenuHomePhabricator

reames (Philip Reames)
User

Projects

User does not belong to any projects.

User Details

User Since
Oct 23 2013, 6:32 PM (354 w, 2 d)

Recent Activity

Thu, Jul 30

reames accepted D81646: MIR Statepoint refactoring. Part 2: Operand folding..
Thu, Jul 30, 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.

Thu, Jul 30, 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).

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

Wed, Jul 29

reames committed rG755f91f12cf0: [Statepoint] Enable cross block relocates w/vreg lowering (authored by reames).
[Statepoint] Enable cross block relocates w/vreg lowering
Wed, Jul 29, 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
Wed, Jul 29, 12:45 PM
reames committed rG8fe2abc190f9: [Statepoint] Consolidate relocation type tracking [NFC] (authored by reames).
[Statepoint] Consolidate relocation type tracking [NFC]
Wed, Jul 29, 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

Wed, Jul 29, 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…
Wed, Jul 29, 9:24 AM
reames closed D84692: [Statepoint] When using the tied def lowering, unconditionally use vregs [almost NFC].
Wed, Jul 29, 9:24 AM · Restricted Project

Mon, Jul 27

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

Address Register related review comments

Mon, Jul 27, 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.
Mon, Jul 27, 12:08 PM · Restricted Project

Sat, Jul 25

reames committed rG55dae9c20ce3: [Statepoints] Style cleanup after 3da1a963 [NFC] (authored by reames).
[Statepoints] Style cleanup after 3da1a963 [NFC]
Sat, Jul 25, 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
Sat, Jul 25, 2:26 PM
reames closed D81648: MIR Statepoint refactoring. Part 4: ISEL changes..
Sat, Jul 25, 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.

Sat, Jul 25, 2:25 PM · Restricted Project

Sat, Jul 11

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.

Sat, Jul 11, 3:26 PM · Restricted Project

Fri, Jul 10

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

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

Fri, Jul 10, 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.

Fri, Jul 10, 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.

Fri, Jul 10, 11:07 AM · Restricted Project

Thu, Jul 9

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

And thank you for doing this.

Thu, Jul 9, 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

Jun 1 2020

reames created D80937: Introduce a "gc-live" bundle for the gc arguments of a statepoint.
Jun 1 2020, 10:45 AM · Restricted Project
reames accepted D80714: [StatepointLowering] Handle UNDEF gc values..

LGTM w/required changes applied before commit. If you disagree on any point, further review needed and LGTM does not apply.

Jun 1 2020, 10:13 AM · Restricted Project
reames requested changes to D80191: MIR Statepoint refactoring: pass GC pointers in VRegs. Part 1/5..

Let's discuss offline.

Jun 1 2020, 10:12 AM · Restricted Project

May 31 2020

reames created D80892: Remove deopt and gc transition arguments from gc.statepoint intrinsic.
May 31 2020, 11:26 AM · Restricted Project

May 30 2020

reames committed rGdfa82f8af446: [Tests] Convert last statepoint lowering tests to bundle format (authored by reames).
[Tests] Convert last statepoint lowering tests to bundle format
May 30 2020, 1:15 PM

May 29 2020

reames requested changes to D80191: MIR Statepoint refactoring: pass GC pointers in VRegs. Part 1/5..

Please pull out a purely NFC patch without any of the first-N-vregs bits which is purely functional. (i.e. replaces the existing target node with the generic node, introduces the node type and wrapper class, but does nothing else.)

May 29 2020, 12:33 PM · Restricted Project
reames accepted D80445: [EarlyCSE] Common gc.relocate calls..

LGTM

May 29 2020, 12:33 PM · Restricted Project
reames added a comment to D80714: [StatepointLowering] Handle UNDEF gc values..

The alternative here is not "do we relocate undef". It's do we let the backend chose arbitrary values for an undef operand. Said more strongly, the compiler is allowed to pick any value it wants for undef, and setting it to an arbitrary constant seems fine from a legality perspective.

May 29 2020, 12:03 PM · Restricted Project
reames added a comment to D80618: Extend InvokeInst !prof branch_weights metadata to unwind branches.

Mostly minor comments on style to help drive review forward. This is a drive by comment, don't wait on me please.

May 29 2020, 12:00 PM · Restricted Project
reames resigned from D43002: Emit S_OBJNAME symbol in CodeView.
May 29 2020, 12:00 PM · Restricted Project, Restricted Project

May 28 2020

reames committed rG66e6b9afa833: [Tests] Migrate more statepoint lowering tests to use operand bundles (authored by reames).
[Tests] Migrate more statepoint lowering tests to use operand bundles
May 28 2020, 8:19 PM
reames committed rG85bf78df654b: [Tests] Update a few more statepoint tests (authored by reames).
[Tests] Update a few more statepoint tests
May 28 2020, 3:27 PM
reames committed rG15000255d18b: [Tests] Remove deopt operands from SafepointIRVerfier tests (authored by reames).
[Tests] Remove deopt operands from SafepointIRVerfier tests
May 28 2020, 2:54 PM
reames committed rG27304b1737a3: [Tests] Switch a few statepoint tests to using operand bundles (authored by reames).
[Tests] Switch a few statepoint tests to using operand bundles
May 28 2020, 2:54 PM
reames committed rG9d065477942f: [Statepoints] Sink routines for grabbing projections to GCStatepointInst [NFC] (authored by reames).
[Statepoints] Sink routines for grabbing projections to GCStatepointInst [NFC]
May 28 2020, 2:19 PM
reames committed rGa0d2fd4a1f78: [Statepoint] Sink actual_args and gc_args to GCStatepointInst [NFC] (authored by reames).
[Statepoint] Sink actual_args and gc_args to GCStatepointInst [NFC]
May 28 2020, 2:19 PM
reames committed rG4d6cda9bdaca: [Statepoint] Use iterate_range.empty [NFC] (authored by reames).
[Statepoint] Use iterate_range.empty [NFC]
May 28 2020, 2:19 PM
reames committed rG58beb76b7bd2: [Statepoint] Convert a few more isStatepoint calls to idiomatic isa/cast (authored by reames).
[Statepoint] Convert a few more isStatepoint calls to idiomatic isa/cast
May 28 2020, 11:36 AM
reames committed rG501aa47ab8fa: [Statepoint] Sink logic about actual callee into GCStatepointInst (authored by reames).
[Statepoint] Sink logic about actual callee into GCStatepointInst
May 28 2020, 11:01 AM
reames committed rG587fa99cfdb7: Default to generating statepoints with deopt and gc-transition bundles if needed (authored by reames).
Default to generating statepoints with deopt and gc-transition bundles if needed
May 28 2020, 10:27 AM
reames closed D80674: Default to generating statepoints with deopt and gc-transition bundles if needed.

Closed via commit 587fa99cfdb7d2a97143ba20ed8e8face57aa01c

May 28 2020, 10:23 AM · Restricted Project
reames added a comment to D80674: Default to generating statepoints with deopt and gc-transition bundles if needed.

LGTM with two notes:

  • Does it makes sense to make GCArgs Optional<> too? Implementation might change later, but you won't need to change interface again.

At the moment, no. Or more accurately, I haven't thought about it enough yet to be comfortable revising the interface. We can always revise later if needed.

May 28 2020, 10:23 AM · Restricted Project
reames closed D80598: Start migrating away from statepoint's inline length prefixed argument bundles.

Closed w/commit 1af3705c7fe23db9d5308bfdf07bfbd04398b895

May 28 2020, 9:54 AM · Restricted Project

May 27 2020

reames committed rG98a87c65a353: [Statepoint] Reduce scope of usage of ImmutableStatepoint (authored by reames).
[Statepoint] Reduce scope of usage of ImmutableStatepoint
May 27 2020, 7:07 PM
reames committed rG87bea912c27c: [Statepoint] Replace uses of isX functions with idiomatic isa<X> (authored by reames).
[Statepoint] Replace uses of isX functions with idiomatic isa<X>
May 27 2020, 6:35 PM
reames committed rG74671d5c1491: Sink first bit of functionality from Statepoint to GCStatepointInst (authored by reames).
Sink first bit of functionality from Statepoint to GCStatepointInst
May 27 2020, 6:35 PM
reames committed rGc94c5bf9cce8: Introduce a GCStatepointInst type analogous to IntrinsicInst subclasses (authored by reames).
Introduce a GCStatepointInst type analogous to IntrinsicInst subclasses
May 27 2020, 5:30 PM
reames updated the diff for D80674: Default to generating statepoints with deopt and gc-transition bundles if needed.

Make a deeper change to the interface to simplify the semantic description of the patch and avoid introducing something we'd have to fix later anyway.

May 27 2020, 4:56 PM · Restricted Project
reames created D80674: Default to generating statepoints with deopt and gc-transition bundles if needed.
May 27 2020, 4:55 PM · Restricted Project
reames committed rG1af3705c7fe2: Start migrating away from statepoint's inline length prefixed argument bundles (authored by reames).
Start migrating away from statepoint's inline length prefixed argument bundles
May 27 2020, 9:45 AM
reames added a comment to D80598: Start migrating away from statepoint's inline length prefixed argument bundles.

LGTM. thanks for working on this! Apart from dropping all of the extra handling code in RS4GC and verifier (which itself is a really good reason for this support btw), I'm assuming this for optimizations which know about the deopt_op bundle (or gc_transition bundle) after relocations are made explicit? i.e., a general robustness patch.

I can't quite parse what you meant with that last sentence.

May 27 2020, 9:43 AM · Restricted Project

May 26 2020

reames committed rG323d85042747: Add self as code owner for SCEV and IndVars (authored by reames).
Add self as code owner for SCEV and IndVars
May 26 2020, 6:01 PM
reames committed rGbed6624ac43b: Split a test file so that most of it can be autogened (authored by reames).
Split a test file so that most of it can be autogened
May 26 2020, 6:01 PM
reames committed rGb90eb0f23b5b: Autogen a couple of test files to make a future diff easier to read (authored by reames).
Autogen a couple of test files to make a future diff easier to read
May 26 2020, 6:01 PM
reames updated the diff for D80598: Start migrating away from statepoint's inline length prefixed argument bundles.

Fix a couple of stylistic points noticed when looking at the web diff

May 26 2020, 4:55 PM · Restricted Project
reames created D80598: Start migrating away from statepoint's inline length prefixed argument bundles.
May 26 2020, 4:23 PM · Restricted Project

May 22 2020

reames resigned from D78203: [VP,Integer,#2] ExpandVectorPredication pass.
May 22 2020, 3:34 PM · Restricted Project, Restricted Project
reames added a comment to D80445: [EarlyCSE] Common gc.relocate calls..

Just to be clear, if the indices are the same, gc.relocates would already be removed right? i.e. the thing you're trying to solve is that the standpoint can have two operands which are the same value and thus two gc.relocates which are equivalent but not textually identical? Just want to make sure I understand the problem properly.

May 22 2020, 3:34 PM · Restricted Project
reames resigned from D80289: [Driver][X86] Support branch align options with LTO.
May 22 2020, 3:34 PM · Restricted Project

May 6 2020

reames added inline comments to D78615: [ValueTracking] Let propagatesPoison support binops/unaryops/cast/etc..
May 6 2020, 2:44 PM · Restricted Project

May 4 2020

reames added a comment to D73181: [SCEV] Use backedge SCEV of PHI only if its input is loop invariant.

This patch bugged me when I first saw it go by; finally got back to looking at it today.

May 4 2020, 10:43 AM · Restricted Project

Apr 17 2020

reames added a comment to D77851: [X86][MC] Make -x86-pad-max-prefix-size compatible with --mc-relax-all.

Has this been reverted? I see discussion of a fix being worked on, but has the change which triggered problems been reverted? I can't tell easily from the review history.

Apr 17 2020, 2:05 PM · Restricted Project

Apr 15 2020

reames committed rG80c46c53bd01: [PoisonChecking] Further clarify file scope comment, and update to match naming… (authored by reames).
[PoisonChecking] Further clarify file scope comment, and update to match naming…
Apr 15 2020, 2:57 PM