reames (Philip Reames)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Sat, Feb 17

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.

Sat, Feb 17, 2:48 PM

Thu, Feb 15

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!

Thu, Feb 15, 11:42 AM

Fri, Feb 2

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

Mon, Jan 29

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

Mon, Jan 29, 4:43 PM

Mon, Jan 22

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.

Mon, Jan 22, 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.

Mon, Jan 22, 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
reames accepted D41675: Remove alignment argument from memcpy/memmove/memset in favour of alignment attributes (Step 1).

LGTM w/minor comment applied.

Jan 17 2018, 6:32 PM
reames committed rL322747: [MDA] Use common code instead of reimplementing same. [NFC].
[MDA] Use common code instead of reimplementing same. [NFC]
Jan 17 2018, 11:59 AM

Jan 12 2018

reames requested changes to D41675: Remove alignment argument from memcpy/memmove/memset in favour of alignment attributes (Step 1).

Much closer to reasonable, likely one or two more iterations.

Jan 12 2018, 9:22 AM
reames requested changes to D41951: [Attributes] Fix crash when attempting to remove alignment from an attribute list/set.

A few minor changes needed, but generally looks close to ready.

Jan 12 2018, 8:58 AM

Jan 11 2018

reames added a comment to D41908: [X86][MMX] Add support for MMX zero vector creation.

Wouldn't we already need/have handling for a zeroinitializer constant of x86_mmx type?

From LangRef: "There are no arrays, vectors or constants of this type."

Should we consider changing that? We can always revise the LangRef to allow constants if that simplifies the implementation.

Jan 11 2018, 11:34 AM

Jan 10 2018

reames requested changes to D41675: Remove alignment argument from memcpy/memmove/memset in favour of alignment attributes (Step 1).

Meta comment: This change is too large *conceptually*. You need to break it apart in a way that the test changes are "obviously correct" because there's no possible change in behaviour. At the moment, it a behaviour change might be lurking in the test changes and that'd never be seen.

Jan 10 2018, 3:13 PM
reames added a comment to D41908: [X86][MMX] Add support for MMX zero vector creation.

Reading along for my own education, feel free to ignore if I'm way off base.

Jan 10 2018, 2:42 PM

Jan 8 2018

reames added a comment to D40055: [SelectionDAG][X86] Explicitly store the scale in the gather/scatter ISD nodes.

This looks like the right approach to me. I think the previously expressed concern was about making the IR representation overly specific to what current hardware supports. This looks largely orthogonal to me and much less confusing as well.

Jan 8 2018, 4:12 PM

Jan 5 2018

reames added a comment to D41761: Introduce llvm.nospeculateload intrinsic.

A design variation on this which may be worth considering is to phrase this as a speculative use barrier. That is, don't include the load at all, simply provide an intrinsic which guarantees that the result of the load (or any other instruction) will not be consumed by a speculative use with potential side-channel visible side effects.

Jan 5 2018, 11:45 AM
reames added a comment to D41723: Introduce the "retpoline" x86 mitigation technique for variant #2 of the speculative execution vulnerabilities disclosed today, specifically identified by CVE-2017-5715, "Branch Target Injection", and is one of the two halves to Spectre...

The only really interesting cases are statepoints and patchpoints. The other cases are only relevant when in the large code model on x86-64 which is pretty much riddled with bugs already. =/

To be explicit, you can ignore statepoints and patchpoints for the moment. As the only major user of this functionality, we'll follow up with patches if needed. We're still in the process of assessing actual risk in our environment, but at the moment it looks like we likely won't need this functionality. (Obviously, subject to change as we learn more.)

Jan 5 2018, 11:19 AM

Jan 3 2018

reames committed rL321764: [PRE] Add a bunch of test cases for LICM-like PRE patterns.
[PRE] Add a bunch of test cases for LICM-like PRE patterns
Jan 3 2018, 2:29 PM

Jan 2 2018

reames added a comment to D41098: [InlineSpiller] Fix a crash due to lack of forward progress from remat.

ping

Jan 2 2018, 2:17 PM
reames accepted D41227: [CGP] Fix Complex addressing mode for offset.

LGTM

Jan 2 2018, 2:16 PM
reames added a comment to D37467: Add a new pass to speculate around PHI nodes with constant (integer) operands when profitable..

Catching up on old review traffic. Nothing critical.

Jan 2 2018, 2:12 PM
reames abandoned D27293: [WIP] Cleanup SplitCSR implementation.

Don't have time to get back to this.

Jan 2 2018, 9:17 AM

Dec 30 2017

reames committed rL321591: 2nd attempt at "fixing" amdgpu tests after r321575​.
2nd attempt at "fixing" amdgpu tests after r321575​
Dec 30 2017, 7:35 PM
reames committed rL321589: Test fix after r321575.
Test fix after r321575
Dec 30 2017, 10:43 AM

Dec 29 2017

reames committed rL321575: [instsimplify] consistently handle undef and out of bound indices for….
[instsimplify] consistently handle undef and out of bound indices for…
Dec 29 2017, 9:55 PM
reames committed rL321573: Add another test case for r321489.
Add another test case for r321489
Dec 29 2017, 8:11 PM
reames committed rL321572: Move tests associated with transforms moved in r321467 .
Move tests associated with transforms moved in r321467
Dec 29 2017, 7:14 PM

Dec 26 2017

reames committed rL321468: [instcombine] add powi(x, 2) -> x * x.
[instcombine] add powi(x, 2) -> x * x
Dec 26 2017, 5:31 PM
reames committed rL321467: Sink a couple of transforms from instcombine into instsimplify..
Sink a couple of transforms from instcombine into instsimplify.
Dec 26 2017, 5:15 PM
reames committed rL321466: [NFC] Extract out a helper function for SimplifyCall(CS, Q).
[NFC] Extract out a helper function for SimplifyCall(CS, Q)
Dec 26 2017, 4:17 PM

Dec 21 2017

reames added inline comments to D41098: [InlineSpiller] Fix a crash due to lack of forward progress from remat.
Dec 21 2017, 11:21 AM
reames updated the diff for D41098: [InlineSpiller] Fix a crash due to lack of forward progress from remat.

Second attempt, this time accounting for sub-registers

Dec 21 2017, 11:18 AM

Dec 17 2017

reames accepted D40808: [RISCV] Implement branch analysis.

Much cleaner, thanks!

Dec 17 2017, 12:02 PM

Dec 13 2017

reames accepted D40807: [RISCV] Support stack frames and offsets up to 32-bits.

Much cleaner, thanks!

Dec 13 2017, 7:10 PM
reames requested changes to D40808: [RISCV] Implement branch analysis.

Adding a machine pass which just called analyzeBranch on each BB and printed the result would be straight-forward if you wanted to cleanup the testing.

Dec 13 2017, 7:05 PM

Dec 12 2017

reames accepted D40634: [CGP] Enable select in complex addr mode.

The code change here is a clear improvement. Given no response from the test case author, LGTM.

Dec 12 2017, 8:57 PM

Dec 11 2017

reames created D41098: [InlineSpiller] Fix a crash due to lack of forward progress from remat.
Dec 11 2017, 4:15 PM

Dec 5 2017

reames added a comment to D39835: [GVN PRE] Clear nsw/nuw for original values in LoadPRE.

What is the resolution of the review? Should I land it?

I'm very strongly of the opinion this patch should not land, ever. I don't believe this change is desirable or necessary. If I'm reading through the history here correctly, what we're worried about is a two phase transform: 1) we replace a load with a phi of previously stored values, and then 2) we replace that phi with a single add instruction. It's specifically that *second* transformation which exposes the worried about miscompile. My argument here is simple: a test case with a *manually written* phi in the same place as our PRE based implementation of FRE inserts one would expose the same miscompile. Given this, we might very well have a bug in whatever bug does the second transform, but there is nothing wrong with the FRE/PRE here.

Dec 5 2017, 1:45 PM

Dec 4 2017

reames added a comment to D39849: [RISCV] Implement prolog and epilog insertion.

small drive by comments.

Dec 4 2017, 7:08 PM
reames requested changes to D40807: [RISCV] Support stack frames and offsets up to 32-bits.

There are a couple of NFC parts that need extracted here before the semantic change can be easily reviewed.

Dec 4 2017, 7:03 PM
reames accepted D39895: [RISCV] MC layer support for the standard RV32D instruction set extension.

LGTM w/changes applied.

Dec 4 2017, 6:49 PM
reames accepted D39893: [RISCV] MC layer support for the standard RV32F instruction set extension.

LGTM w/comments addressed before submit.

Dec 4 2017, 6:39 PM

Dec 1 2017

reames committed rL319583: [IndVars] Fix a bug introduced in r317012.
[IndVars] Fix a bug introduced in r317012
Dec 1 2017, 12:57 PM

Nov 27 2017

reames requested changes to D37579: [InstCombine] Fix PR21780 Expansion of 256 bit vector loads fails to fold into shuffles.

Initial round of comments focused on the metadata semantics only. I completely skipped the instcombine changes.

Nov 27 2017, 11:28 AM

Nov 21 2017

reames accepted D39453: [SCEV] Strengthen variance condition in calculateLoopDisposition.

LGTM

Nov 21 2017, 8:21 PM

Nov 14 2017

reames requested changes to D39453: [SCEV] Strengthen variance condition in calculateLoopDisposition.

Max and I discussed and realized this comes down to simply needing a much simpler and straight-forward comment. :) If AR is not defined on entry to L, it can't be LoopInvariant with respect to L. The distinction between L being a parent of AR->loop vs a sibling vs a nephew, cousin, etc... really doesn't contribute anything here.

Nov 14 2017, 9:05 PM
reames accepted D39236: [SCEV][NFC] Introduce isSafeToExpandAt function to SCEVExpander.

LGTM

Nov 14 2017, 8:31 PM

Nov 7 2017

reames added a comment to D39743: [ValueLattice] Add CompactValueLatticeElement..

Structural suggestion: pick either of the two subtasks and implement them in isolation. That is *either* replace the ConstantRange with an owned ConstantRange allocation and implement the tagged pointer OR implement the ConstantRange folding set. Doing both in one change is needlessly complicated. Once one is done, the other follows more naturally. I'd suggest the first then the second, but either order is defensible.

Nov 7 2017, 4:16 PM
reames added a reviewer for D39743: [ValueLattice] Add CompactValueLatticeElement.: anna.
Nov 7 2017, 4:16 PM
reames accepted D39234: [IRCE] Fix SCEVExpander's usage in IRCE.

LGTM once dependent changes land.

Nov 7 2017, 4:05 PM
reames requested changes to D39236: [SCEV][NFC] Introduce isSafeToExpandAt function to SCEVExpander.

relatively minor code comments.

Nov 7 2017, 4:03 PM
reames added a comment to D39453: [SCEV] Strengthen variance condition in calculateLoopDisposition.

Max, I don't quite follow your discussion enough to know for sure, but I suspect there's something wrong with your framing here. A induction variable in one loop may be loop invariant with respect to sibling loop. That's normal. Is there maybe a subcase here? Or are you looking at flawed assumption in caller code?

Nov 7 2017, 3:58 PM
reames accepted D39589: [IRCE] Remove folding of two range checks into RANGE_CHECK_BOTH.

LGTM, but please land a day or two separate from any other change. i.e. if this exposes problems (pass ordering?) let's make that obvious.

Nov 7 2017, 3:50 PM
reames accepted D39590: [IRCE][NFC] Make range check's End a non-null SCEV.

LGTM once dependent revisions have been in tree for a couple of days.

Nov 7 2017, 3:46 PM

Nov 1 2017

reames committed rL317116: Revert 317016 and 317048.
Revert 317016 and 317048
Nov 1 2017, 12:49 PM
reames added a reverting commit for rL317048: [SimplifyIndVar] Inline makIVComparisonInvariant to eleminate code duplication…: rL317116: Revert 317016 and 317048.
Nov 1 2017, 12:49 PM
reames added a reverting commit for rL317016: [IndVarSimplify] Extract wrapper around SE-.isLoopInvariantPredicate [NFC]: rL317116: Revert 317016 and 317048.
Nov 1 2017, 12:49 PM

Oct 31 2017

reames committed rL317048: [SimplifyIndVar] Inline makIVComparisonInvariant to eleminate code duplication….
[SimplifyIndVar] Inline makIVComparisonInvariant to eleminate code duplication…
Oct 31 2017, 3:56 PM
reames committed rL317016: [IndVarSimplify] Extract wrapper around SE-.isLoopInvariantPredicate [NFC].
[IndVarSimplify] Extract wrapper around SE-.isLoopInvariantPredicate [NFC]
Oct 31 2017, 11:05 AM
reames committed rL317012: [IndVarSimplify] Simplify code using a dictionary.
[IndVarSimplify] Simplify code using a dictionary
Oct 31 2017, 10:07 AM

Oct 30 2017

reames committed rL316976: [IndVarSimplify] Simplify code using preheader assumption.
[IndVarSimplify] Simplify code using preheader assumption
Oct 30 2017, 10:17 PM
reames committed rL316974: [SimplifyIndVar] Extract out invariant expression handling.
[SimplifyIndVar] Extract out invariant expression handling
Oct 30 2017, 9:19 PM
reames accepted D37460: [GVN] Prevent LoadPRE from hoisting across instructions that don't pass control flow to successors.

LGTM

Oct 30 2017, 9:14 PM
reames committed rL316968: Undo accidental commit.
Undo accidental commit
Oct 30 2017, 5:04 PM
reames committed rL316967: [CGP] Fix crash on i96 bit multiply.
[CGP] Fix crash on i96 bit multiply
Oct 30 2017, 5:00 PM
reames requested changes to D37460: [GVN] Prevent LoadPRE from hoisting across instructions that don't pass control flow to successors.
Oct 30 2017, 10:47 AM
reames accepted D39414: [IRCE][NFC] Rename fields of InductiveRangeCheck.

LGTM

Oct 30 2017, 10:40 AM

Oct 29 2017

reames added inline comments to D37460: [GVN] Prevent LoadPRE from hoisting across instructions that don't pass control flow to successors.
Oct 29 2017, 11:28 PM

Oct 27 2017

reames requested changes to D37460: [GVN] Prevent LoadPRE from hoisting across instructions that don't pass control flow to successors.

Ok, once more very close. Thank you for working through the issues raised and splitting out the various bits. Now that's done, one more round of updates and we should be good for a resubmit.

Oct 27 2017, 9:51 AM
reames accepted D39369: [GVN][NFC] Mark instruction for deletion instead of immediate erasing in LoadPRE.

LGTM w/comment applied.

Oct 27 2017, 9:35 AM

Oct 26 2017

reames committed rL316709: [SimplifyIndVars] Shorten code by using SCEV helper [NFC].
[SimplifyIndVars] Shorten code by using SCEV helper [NFC]
Oct 26 2017, 3:02 PM
reames added inline comments to D38944: [GVN] Handle removal of first implicit CF instruction correctly.
Oct 26 2017, 2:50 PM
reames committed rL316699: [LICM] Restructure implicit exit handling to be more clear [NFCI].
[LICM] Restructure implicit exit handling to be more clear [NFCI]
Oct 26 2017, 2:00 PM
reames requested changes to D38944: [GVN] Handle removal of first implicit CF instruction correctly.
Oct 26 2017, 10:18 AM
reames requested changes to D39320: [IRCE] Ensure that expanded exit values are available in loop's preheader.
Oct 26 2017, 9:28 AM

Oct 23 2017

reames added inline comments to D38581: [IRCE] Fix intersection between signed and unsigned ranges.
Oct 23 2017, 1:45 PM

Oct 20 2017

reames accepted D39103: [RISCV 12.5/n] Codegen support for memory operations on global addresses.

LGTM

Oct 20 2017, 4:03 PM
reames accepted D29934: [RISCV 12/n] Codegen support for memory operations.

Thanks for splitting. Tracking the pieces is now much easier.

Oct 20 2017, 3:57 PM
reames accepted D39101: [RISCV 11.5/n] Codegen support for materializing constants.

LGTM

Oct 20 2017, 3:56 PM

Oct 18 2017

reames requested changes to D29934: [RISCV 12/n] Codegen support for memory operations.
Oct 18 2017, 4:58 PM
reames accepted D29938: [RISCV 16/n] Support and tests for a variety of additional LLVM IR constructs.

LGTM w/identified issues fixed before submission.

Oct 18 2017, 4:53 PM

Oct 16 2017

reames closed D38987: [GVN] Revert rL315440.

Sending include/llvm/Transforms/Scalar/GVN.h
Sending lib/Transforms/Scalar/GVN.cpp
Sending test/Transforms/GVN/PRE/pre-load.ll
Transmitting file data ...done
Committing transaction...
Committed revision 315974.

Oct 16 2017, 11:21 PM
reames accepted D38987: [GVN] Revert rL315440.
Oct 16 2017, 11:21 PM
reames committed rL315974: Revert 315440 on behalf of mkazantsev.
Revert 315440 on behalf of mkazantsev
Oct 16 2017, 11:21 PM
reames added a reverting commit for rL315440: [GVN] Prevent LoadPRE from hoisting across instructions that don't pass control…: rL315974: Revert 315440 on behalf of mkazantsev.
Oct 16 2017, 11:21 PM

Oct 10 2017

reames requested changes to D35858: [RFC] [LLVM] [LazyValueInfo] Introduce getRecurringEdgeValue to handle simple recurrence.

Pending update per last comment, just getting this off review queue.

Oct 10 2017, 12:06 PM
reames accepted D37460: [GVN] Prevent LoadPRE from hoisting across instructions that don't pass control flow to successors.

LGTM

Oct 10 2017, 12:04 PM
reames accepted D38392: Disallow sinking of unordered atomic loads into loops.

LGTM

Oct 10 2017, 12:00 PM

Oct 4 2017

reames added inline comments to D38529: [IRCE] Temporarily disable unsigned latch conditions by default.
Oct 4 2017, 10:44 AM

Oct 3 2017

reames accepted D38529: [IRCE] Temporarily disable unsigned latch conditions by default.

LGTM w/the following changes made: add a test case which demonstrates the miscompile and update the description to reference the original problematic commit.

Oct 3 2017, 10:15 PM
reames added inline comments to D37460: [GVN] Prevent LoadPRE from hoisting across instructions that don't pass control flow to successors.
Oct 3 2017, 9:57 AM

Oct 2 2017

reames updated subscribers of D29933: [RISCV 11/n] Initial codegen support for ALU operations.
Oct 2 2017, 8:54 PM
reames accepted D29933: [RISCV 11/n] Initial codegen support for ALU operations.

LGTM so as to unblock patches. I don't see anything obviously wrong with these patches though I might be missing details as this is an area I'm not hugely familiar with.

Oct 2 2017, 8:51 PM
reames requested changes to D37460: [GVN] Prevent LoadPRE from hoisting across instructions that don't pass control flow to successors.
  • List Item

The loadPRE part of this seems very close to being ready to submit.

Oct 2 2017, 8:21 PM