reames (Philip Reames)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Tue, Apr 10

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.

Tue, Apr 10, 4:16 PM

Thu, Mar 29

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
Thu, Mar 29, 1:35 PM
reames committed rL328816: Fix an accidental circular dependence.
Fix an accidental circular dependence
Thu, Mar 29, 12:25 PM

Fri, Mar 23

reames committed rL328387: [GuardWidening] Group code by class [NFC].
[GuardWidening] Group code by class [NFC]
Fri, Mar 23, 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
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