reames (Philip Reames)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Thu, Jul 5

reames added a comment to D44160: [GVN] Don't use the eliminated load as an available value in phi construction.

Sorry, thought I'd reviewed this. I apparently didn't hit submit.

Thu, Jul 5, 11:32 AM
reames added a comment to D48768: [X86] When have BMI2, prefer shifts to clear low/high bits, rather than variable mask..

This specifically looks for (and (shl -1, X), Y) or (and (shr -1, X), Y). If Y in either of those is a constant, I think we would have rewritten the LHS side of the shift already and removed the 'and' entirely.

That was indeed what I was missing. Thanks for the clarification.

Thu, Jul 5, 10:45 AM

Wed, Jul 4

reames added a comment to D48768: [X86] When have BMI2, prefer shifts to clear low/high bits, rather than variable mask..

I may be missing something but isn't this a pessimization for the case where the mask is an immediate that can be directly encoded into the and instruction? Why should we prefer dual shifts over something like an AND RAX, 0xfff?

Wed, Jul 4, 11:07 AM

Wed, Jun 27

reames added a comment to D45162: [EarlyCSE] Add debug counter for debugging mis-optimizations. NFC..

ping! response needed.

Wed, Jun 27, 11:01 AM

Wed, Jun 20

reames added a comment to D46706: [PM/LoopUnswitch] Support partial trivial unswitching..

Sorry for being late to the party, but a couple of optional post commit style comments for possible follow up. Nothing major, just ideas on how to share code and reduce a possible ordering sensitivity.

Wed, Jun 20, 3:54 PM

Tue, Jun 19

reames committed rL335091: Add more test cases for deopt-operands via regalloc.
Add more test cases for deopt-operands via regalloc
Tue, Jun 19, 7:48 PM
reames added a comment to D41098: [InlineSpiller] Fix a crash due to lack of forward progress from remat.

I went ahead and submitted the workaround, but I want to record one other idea I tried. It didn't work out, but I'm not sure if that's due to something fundamental, or if I just had a non-obvious bug.

Tue, Jun 19, 2:32 PM
reames committed rL335077: [InlineSpiller] Fix a crash due to lack of forward progress from remat….
[InlineSpiller] Fix a crash due to lack of forward progress from remat…
Tue, Jun 19, 2:24 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Tue, Jun 19, 2:24 PM

Jun 13 2018

reames added a comment to D47928: [SimplifyIndVars] Eliminate redundant truncs.

Thinking about this further, I see another opportunity for a follow on improvement, sketched below in comments.

Jun 13 2018, 11:20 AM
reames accepted D47928: [SimplifyIndVars] Eliminate redundant truncs.

LGTM w/comments addressed before landing.

Jun 13 2018, 10:57 AM
reames accepted D47574: [EarlyCSE] Propagate conditions of AND and OR instructions.

Current patch LGTM.

Jun 13 2018, 10:35 AM

Jun 12 2018

reames updated the diff for D41098: [InlineSpiller] Fix a crash due to lack of forward progress from remat.

Quentin, I read over your suggested approach, but honestly, I didn't entirely follow. Rather than going through another round with an approach that I clearly don't understand, I'd like to go with a quick and dirty hack for the moment if you don't mind. Are you okay with us solving this just for STATEPOINTs at the moment by essentially just disabling remats for STATEPOINT uses?

Jun 12 2018, 3:57 PM

Jun 11 2018

reames added inline comments to D47928: [SimplifyIndVars] Eliminate redundant truncs.
Jun 11 2018, 6:30 PM

May 31 2018

reames added a comment to D45162: [EarlyCSE] Add debug counter for debugging mis-optimizations. NFC..

This appears to have landed without review. What's going on here?

May 31 2018, 1:13 PM

May 21 2018

reames added a comment to D47113: [CVP] Teach CorrelatedValuePropagation to reduce the width of lshr instruction..

Minor code/style comments only. Leaving the broader discussion to engaged parties.

May 21 2018, 9:49 AM

May 17 2018

reames added a comment to D46996: [LICM] Extend the MustExecute scope.

Seems reasonable, go for it.

May 17 2018, 8:46 PM
reames requested changes to D46866: [EarlyCSE] Avoid a poorly defined instruction comparison.
May 17 2018, 10:06 AM
reames accepted D47011: SafepointIRVerifier is made unreachable block tolerant.

LGTM

May 17 2018, 9:55 AM
reames accepted D46996: [LICM] Extend the MustExecute scope.

LGTM w/minor changes

May 17 2018, 9:05 AM

May 10 2018

reames accepted D46658: [InstCombine] Unify handling of atomic memtransfer with non-atomic memtransfer.

LGTM

May 10 2018, 3:02 PM
reames accepted D46660: [InstCombine] Handle atomic memset in the same way as regular memset.

LGTM

May 10 2018, 2:53 PM

May 9 2018

reames added a comment to D45150: Less conservative LoopSafetyInfo for headers.

We created an abstraction for the ordered basic block caching that works and is used in a few passes.
Transforms/Utils/OrderedInstructions.h

I suspect it's in a bad place for people to find it, and should be next to OrderedBasicBlock (or just make OrderedBasicBlock a hidden implementation detail, since all uses i can find really are doing it for multiple BB's anyway)

Hm, I hadn't been aware of that myself. Will have to take a look.

May 9 2018, 8:31 PM
reames committed rL331944: [Inscombine] fix a signedness warning which broke -Werror builds.
[Inscombine] fix a signedness warning which broke -Werror builds
May 9 2018, 5:09 PM
reames requested changes to D45150: Less conservative LoopSafetyInfo for headers.

Specifically NOT okay to land as is. The quadratic complexity concern is a real one and the window chosen here is very likely too large.

May 9 2018, 5:08 PM
reames added inline comments to D44160: [GVN] Don't use the eliminated load as an available value in phi construction.
May 9 2018, 4:55 PM
reames accepted D44892: Add PerfJITEventListener for perf profiling support..

LGTM w/comments addressed before submission.

May 9 2018, 4:39 PM
reames requested changes to D45581: Add a LocationSize type to represent the size of MemoryLocations.

Ok, I think you missed my point in the other review. The entire idea was to have something which was so obviously correct it didn't need further review. Specifically, to avoid waiting for me or anyone else to review it. Unfortunately, we're now in the worst of both worlds: this change does not appear to be fully NFC and we had a long review delay.

May 9 2018, 4:23 PM
reames committed rL331935: [InstCombine] Widen guards with conditions between.
[InstCombine] Widen guards with conditions between
May 9 2018, 4:00 PM
reames closed D46203: [InstCombine] Widen guards with conditions between.
May 9 2018, 4:00 PM

May 4 2018

reames committed rL331557: [LICM] Compute a must execute property for the prefix of the header as we go.
[LICM] Compute a must execute property for the prefix of the header as we go
May 4 2018, 2:41 PM
reames closed D46211: [LICM] Compute a must execute property for the prefix of the header as we go.

Committed revision 331557.

May 4 2018, 2:41 PM
reames added a comment to D46203: [InstCombine] Widen guards with conditions between.

I would suggest a slightly different approach to handle such situations. Whenever we see consecutive guard followed by an instruction that is safe to speculate, we can just swap them. Doing so, we will collect all guards in one continuous sequence after all speculable code. And then we just fold consecutive pairs. What do you think about it?

I thought about this approach and I agree it's interesting. It does have one serious problem I haven't quite figured out yet though which is that hoisting above the guard changes placement in the final lowered IR. If we'd left it where it was, we might have been able to *sink* the instruction into a block with the only user.

May 4 2018, 2:36 PM
reames added inline comments to D46211: [LICM] Compute a must execute property for the prefix of the header as we go.
May 4 2018, 12:43 PM

May 1 2018

reames added a comment to D46240: [llvm] Removing writeonly and readnone incompatibility..

I am working to enable functions that only write to memory to be hoisted out of loops and to combined by EarlyCSE, more specifically I am working to mark the math functions that only write to errno as writeonly then have them handled as writeonly functions.

You'll need something a bit stronger than an aliasing property. You're going to specifically need to model *what* is written to errno. At the moment, we don't have a good way to model a function which writes only to one specific global (errno) or to model what those writes are.

I'm not sure that's true. If a function only writes, then what it writes must always be a function only of its arguments. So if I have:

foo(int, int*) writeonly;
 
foo(a, b);
foo(a, b);

then, I think, we can CSE these calls to foo. foo might write to its second argument somewhere, and might write to globals, but since the arguments are the same (and it doesn't read any state otherwise), it must write the same thing each time.

Good point. I agree this works and likely forms a good basis for what the errno case.

May 1 2018, 8:56 AM
reames requested changes to D46279: [LVI] Remove an assert on case which could happen in LazyValueInfo..

The assertion is checking the a value is not live-in to the entry block unless it is a global. That assert is correct. From your test case, it looks like you've actually found a place where we recurse on a non-local search we shouldn't have. (i.e. a local add with two constant operands should never trigger the backwards walk.)

May 1 2018, 8:51 AM

Apr 30 2018

reames added a comment to D46240: [llvm] Removing writeonly and readnone incompatibility..

I am working to enable functions that only write to memory to be hoisted out of loops and to combined by EarlyCSE, more specifically I am working to mark the math functions that only write to errno as writeonly then have them handled as writeonly functions.

You'll need something a bit stronger than an aliasing property. You're going to specifically need to model *what* is written to errno. At the moment, we don't have a good way to model a function which writes only to one specific global (errno) or to model what those writes are.

Apr 30 2018, 7:38 PM
reames requested changes to D46240: [llvm] Removing writeonly and readnone incompatibility..

This patch makes no sense. The LangRef text is completely reasonable as far as I can tell. What is your actual intent here? What problem are you trying to solve?

Apr 30 2018, 11:53 AM

Apr 27 2018

reames committed rL331094: [LoopGuardWidening] Make PostDomTree optional.
[LoopGuardWidening] Make PostDomTree optional
Apr 27 2018, 4:21 PM
reames added inline comments to D46203: [InstCombine] Widen guards with conditions between.
Apr 27 2018, 2:58 PM
reames created D46212: [WIP][LICM] Hoisting of guards, assumes, and invariant_starts.
Apr 27 2018, 2:49 PM
reames created D46211: [LICM] Compute a must execute property for the prefix of the header as we go.
Apr 27 2018, 2:23 PM
reames committed rL331080: [LICM] Reduce nesting with an early return [NFC].
[LICM] Reduce nesting with an early return [NFC]
Apr 27 2018, 2:02 PM
reames committed rL331079: [MustExecute/LICM] Special case first instruction in throwing header.
[MustExecute/LICM] Special case first instruction in throwing header
Apr 27 2018, 1:47 PM
reames created D46203: [InstCombine] Widen guards with conditions between.
Apr 27 2018, 12:04 PM
reames committed rL331061: [GuardWidening] Add some clarifying comments about heuristics [NFC].
[GuardWidening] Add some clarifying comments about heuristics [NFC]
Apr 27 2018, 10:45 AM
reames committed rL331060: [LoopGuardWidening] Split out a loop pass version of GuardWidening.
[LoopGuardWidening] Split out a loop pass version of GuardWidening
Apr 27 2018, 10:32 AM

Apr 10 2018

reames added a comment to D44748: Track whether the size of a MemoryLocation is precise.

I am concerned that you doing too many things in one patch and that the mix of refactoring and functional changes may contain hard to spot bugs. In particular, the absence of hasValue checks in various updated logic worries me.

Apr 10 2018, 4:16 PM

Mar 29 2018

reames committed rL328822: [NFC][LICM] Rearrange checks to have the cheap bail out first.
[NFC][LICM] Rearrange checks to have the cheap bail out first
Mar 29 2018, 1:35 PM
reames committed rL328816: Fix an accidental circular dependence.
Fix an accidental circular dependence
Mar 29 2018, 12:25 PM

Mar 23 2018

reames committed rL328387: [GuardWidening] Group code by class [NFC].
[GuardWidening] Group code by class [NFC]
Mar 23 2018, 4:44 PM

Mar 20 2018

reames committed rL328058: [MustExecute] Shwo the effect of using full loop info variant.
[MustExecute] Shwo the effect of using full loop info variant
Mar 20 2018, 4:03 PM
reames committed rL328056: [MustExecute] Add simplest possible test for LoopSafetyOnfo.
[MustExecute] Add simplest possible test for LoopSafetyOnfo
Mar 20 2018, 3:59 PM
reames committed rL328055: [MustExecute] Move isGuaranteedToExecute and related rourtines to Analysis.
[MustExecute] Move isGuaranteedToExecute and related rourtines to Analysis
Mar 20 2018, 3:48 PM
reames committed rL328015: [MustExecute] Use the annotation style printer.
[MustExecute] Use the annotation style printer
Mar 20 2018, 11:46 AM
reames committed rL328004: Add an analysis printer for must execute reasoning.
Add an analysis printer for must execute reasoning
Mar 20 2018, 10:12 AM
reames closed D44524: Add an analysis printer for must execute reasoning.
Mar 20 2018, 10:11 AM

Mar 16 2018

reames updated the diff for D44524: Add an analysis printer for must execute reasoning.

Fix Anna's comments.

Mar 16 2018, 2:50 PM
reames added inline comments to D44524: Add an analysis printer for must execute reasoning.
Mar 16 2018, 1:50 PM
reames added inline comments to D44524: Add an analysis printer for must execute reasoning.
Mar 16 2018, 11:00 AM
reames added a comment to D44524: Add an analysis printer for must execute reasoning.

Just to note, other printer passes like print-memoryssa, print-lvi, print-predicate-info, we print the annotations as comments just above the IR (using the asm printer interface). It's easier to see which instruction we are referring to and quickly spot the loops they were part of , *if loops are properly annotated in the IR*. It would be useful when debugging large code base IR and spotting the missing cases.
IMO, the missing cases may be hard to spot in this printer. One workaround is to print the number of loops where it's not guaranteed to execute. That can be a separate add on after landing this as well.

From a testing perspective of the utility, this version works well for me.

This is a good idea. I'd copied the basic structure of this from the mem deref printer, but I like the annotation version better. Mind if I submit this one (so that I can start getting some basic testing around this code), and then return to the annotation as a follow on? I'll change over mem-deref while I'm at it.

Mar 16 2018, 9:41 AM
reames committed rL327722: [LICM/mustexec] Extend first iteration must execute logic to fcmps.
[LICM/mustexec] Extend first iteration must execute logic to fcmps
Mar 16 2018, 9:36 AM
reames closed D44542: [LICM/mustexec] Extend first iteration must exexute logic to fcmps.
Mar 16 2018, 9:36 AM

Mar 15 2018

reames updated the summary of D44542: [LICM/mustexec] Extend first iteration must exexute logic to fcmps.
Mar 15 2018, 2:23 PM
reames created D44542: [LICM/mustexec] Extend first iteration must exexute logic to fcmps.
Mar 15 2018, 2:23 PM
reames committed rL327664: [LICM] Ignore exits provably not taken on first iteration when computing must….
[LICM] Ignore exits provably not taken on first iteration when computing must…
Mar 15 2018, 2:08 PM
reames closed D44287: [LICM] Ignore exits provably not taken on first iteration when computing must execute.
Mar 15 2018, 2:07 PM
reames added a comment to D44497: [EarlyCSE] Reuse invariant scopes for invariant load.

Note: The variant I committed turned out to be less powerful in one particular case than the reviewed code. I spotted that after submission and fixed it in revision 327655. The fix also improves the invariant.start case which had an analogous problem.

Mar 15 2018, 11:16 AM
reames committed rL327655: [EarlyCSE] Don't hide earler invariant.scopes.
[EarlyCSE] Don't hide earler invariant.scopes
Mar 15 2018, 11:15 AM
reames committed rL327646: [EarlyCSE] Reuse invariant scopes for invariant load.
[EarlyCSE] Reuse invariant scopes for invariant load
Mar 15 2018, 10:32 AM
reames closed D44497: [EarlyCSE] Reuse invariant scopes for invariant load.
Mar 15 2018, 10:32 AM
reames created D44524: Add an analysis printer for must execute reasoning.
Mar 15 2018, 9:22 AM

Mar 14 2018

reames requested changes to D44244: [LLVM] Add -git-commit-after-all option.

It was pointed out that upstream support for dumping the whole module already exists. It's spelled as: -print-after-all -print-module-scope

Mar 14 2018, 3:26 PM
reames updated the diff for D44287: [LICM] Ignore exits provably not taken on first iteration when computing must execute.

address Max's comment. I didn't manage to actual test the multiple latch case through LICM though, see attempt and comment.

Mar 14 2018, 3:20 PM
reames created D44497: [EarlyCSE] Reuse invariant scopes for invariant load.
Mar 14 2018, 3:02 PM
reames committed rL327577: [EarlyCSE] Exploit open ended invariant.start scopes.
[EarlyCSE] Exploit open ended invariant.start scopes
Mar 14 2018, 2:37 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Mar 14 2018, 2:37 PM
reames added inline comments to D44287: [LICM] Ignore exits provably not taken on first iteration when computing must execute.
Mar 14 2018, 11:09 AM

Mar 9 2018

reames updated the diff for D44287: [LICM] Ignore exits provably not taken on first iteration when computing must execute.

address Anna's comments

Mar 9 2018, 3:28 PM
reames added inline comments to D44287: [LICM] Ignore exits provably not taken on first iteration when computing must execute.
Mar 9 2018, 3:22 PM
reames updated the diff for D43716: [EarlyCSE] Exploit open ended invariant.start scopes.

Clarify a couple of comments.

Mar 9 2018, 3:21 PM
reames added inline comments to D43716: [EarlyCSE] Exploit open ended invariant.start scopes.
Mar 9 2018, 3:21 PM
reames updated the diff for D44288: [WIP][LICM] Extend must execute to path taken on first iteration.
Mar 9 2018, 9:07 AM

Mar 8 2018

reames created D44288: [WIP][LICM] Extend must execute to path taken on first iteration.
Mar 8 2018, 10:38 PM
reames created D44287: [LICM] Ignore exits provably not taken on first iteration when computing must execute.
Mar 8 2018, 9:36 PM
reames committed rL327065: [NFC] Factor out a helper function for checking if a block has a potential….
[NFC] Factor out a helper function for checking if a block has a potential…
Mar 8 2018, 1:28 PM

Feb 23 2018

reames created D43716: [EarlyCSE] Exploit open ended invariant.start scopes.
Feb 23 2018, 8:32 PM

Feb 17 2018

reames accepted D43081: [AlignmentFromAssumptions] Set source and dest alignments of memory intrinsiscs separately.

LGTM provided that the backend changes to take the min of the two have already landed.

Feb 17 2018, 2:48 PM

Feb 15 2018

reames added a comment to D43201: [X86] Only reorder srl/and on last DAG combiner run.

(Beyond the scope of this patch…) I wish LLVM or the x86 backend could change its instruction selection on the fly. Then it could prefer "reasonable density" (not microcoded) by default and "throughput" in loops (or otherwise obvious hot paths). This would probably yield the best overall/pragmatic performance.

Just to second this, I think there's a lot of potential gain to be had by being able to select cold blocks for size and hot blocks for speed. As stated, definitely off topic for this review though!

Feb 15 2018, 11:42 AM

Feb 2 2018

reames requested changes to D42850: [docs] Add guidance on duplicating doc comments to CodingStandards.
Feb 2 2018, 2:12 PM

Jan 29 2018

reames added a comment to D42646: [X86] Avoid using high register trick for test instruction.

Is it worth considering doing this as a late peephole? (i.e. after register allocation if the register happens to be appropriate?)

Jan 29 2018, 4:43 PM

Jan 22 2018

reames added a comment to D42251: [globalisel][legalizer] Adapt LegalizerInfo to support inter-type dependencies and other things..

I generally like the direction of the API. Again, not really a qualified reviewer, but I added a bunch of small comments on readability of the API choices from a non-expert. Making this stuff more discoverable would be a major plus and this feels like a potential major step in that direction.

Jan 22 2018, 4:56 PM
reames resigned from D42244: [globalisel] Introduce LegalityQuery to better encapsulate the legalizer decisions. NFC..

Not really a qualified reviewer for this, but I'll comment this reads more cleanly to someone unfamiliar with the details of the code.

Jan 22 2018, 4:21 PM

Jan 20 2018

reames committed rL323056: [DSE] Factor out common code [NFC].
[DSE] Factor out common code [NFC]
Jan 20 2018, 6:12 PM
reames committed rL323055: [DSE] Minor rename for clarity sake [NFC].
[DSE] Minor rename for clarity sake [NFC]
Jan 20 2018, 5:46 PM
reames accepted D41903: [ValueLattice] Use union to shave off ptr size bytes from elements..

Also, LGTM since I did have time to look through this time. :)

Jan 20 2018, 10:59 AM
reames added a comment to D41903: [ValueLattice] Use union to shave off ptr size bytes from elements..

LGTM after your changes but please wait for Philip.

In a case like this where I only had minor comments, please don't wait for me. If davade things this is good to go, run with it. I tend to be very busy and blocking anyone's forward progress on my availability is a bad idea.

Jan 20 2018, 10:56 AM

Jan 18 2018

reames accepted D42270: [ValueLattice] Use getters instead of direct accesses (NFC)..
Jan 18 2018, 4:57 PM

Jan 17 2018

reames requested changes to D41903: [ValueLattice] Use union to shave off ptr size bytes from elements..

Bunch of minor comments, but once those addressed, likely good to go.

Jan 17 2018, 6:55 PM
reames requested changes to D39743: [ValueLattice] Add CompactValueLatticeElement..

Marking to get off my queue.

Jan 17 2018, 6:55 PM