Page MenuHomePhabricator

reames (Philip Reames)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Fri, Aug 23

reames created D66688: [LoopVectorize] Leverage speculation safety to avoid masked.loads.
Fri, Aug 23, 3:05 PM · Restricted Project
reames added a comment to D66608: [InstCombine] icmp eq/ne (gep inbounds P, Idx..), null -> icmp eq/ne P, null.

A vector extension of this is now posted for review: https://reviews.llvm.org/D66671

Fri, Aug 23, 11:48 AM · Restricted Project
reames created D66671: [InstCombine] icmp eq/ne (gep inbounds P, Idx..), null -> icmp eq/ne P, null for vectors.
Fri, Aug 23, 11:47 AM · Restricted Project
reames committed rG9cb059fdcc03: Fix a bug in just submitted rL369789 (authored by reames).
Fix a bug in just submitted rL369789
Fri, Aug 23, 11:28 AM
reames committed rL369795: Fix a bug in just submitted rL369789.
Fix a bug in just submitted rL369789
Fri, Aug 23, 11:27 AM
reames added a comment to D66608: [InstCombine] icmp eq/ne (gep inbounds P, Idx..), null -> icmp eq/ne P, null.

I noticed a bug in what got committed, and fixed that in rL369795.

Fri, Aug 23, 11:27 AM · Restricted Project
reames abandoned D64533: [IndVars] Special case the problematic (gep inbounds p, iv == nullptr) problem (pr42357) .

Alternate change landed, so patch abandoned.

Fri, Aug 23, 11:10 AM
reames committed rG5b02cfa0b3c2: [InstCombine] icmp eq/ne (gep inbounds P, Idx..), null -> icmp eq/ne P, null (authored by reames).
[InstCombine] icmp eq/ne (gep inbounds P, Idx..), null -> icmp eq/ne P, null
Fri, Aug 23, 11:04 AM
reames committed rL369789: [InstCombine] icmp eq/ne (gep inbounds P, Idx..), null -> icmp eq/ne P, null.
[InstCombine] icmp eq/ne (gep inbounds P, Idx..), null -> icmp eq/ne P, null
Fri, Aug 23, 11:04 AM
reames closed D66608: [InstCombine] icmp eq/ne (gep inbounds P, Idx..), null -> icmp eq/ne P, null.
Fri, Aug 23, 11:03 AM · Restricted Project
reames added a comment to D66309: Introduce infrastructure for an incremental port of SelectionDAG atomic load/store handling.

ping

Fri, Aug 23, 10:36 AM · Restricted Project

Thu, Aug 22

reames committed rG2a52583d6700: [IndVars] Fix a bug noticed by inspection (authored by reames).
[IndVars] Fix a bug noticed by inspection
Thu, Aug 22, 9:04 PM
reames committed rL369730: [IndVars] Fix a bug noticed by inspection.
[IndVars] Fix a bug noticed by inspection
Thu, Aug 22, 9:02 PM
reames updated the diff for D66608: [InstCombine] icmp eq/ne (gep inbounds P, Idx..), null -> icmp eq/ne P, null.

When I went to add the suggested size-0 type bailout, I realized the existing code was correct for this case. I added tests and a comment to show this.

Thu, Aug 22, 8:40 PM · Restricted Project
reames planned changes to D66608: [InstCombine] icmp eq/ne (gep inbounds P, Idx..), null -> icmp eq/ne P, null.

As mentioned on the previous review, do we need to handle getelementptr inbounds {}, {}* %p, i32 %idx? That is, an inbounds GEP on a zero-sized type, which may be null for a non-zero index. It looks like this case is not handled in the existing code either though, so possibly this construction isn't legal (though I don't see anything to that effect in langref)?

I'm happy to add another bailout. To be honest, the exact semantics of unsized types are not all clear to me.

Thu, Aug 22, 4:47 PM · Restricted Project
reames retitled D66608: [InstCombine] icmp eq/ne (gep inbounds P, Idx..), null -> icmp eq/ne P, null from [InstCombine] icmp eq/ne (gep P, Idx..), null -> icmp eq/ne P, null to [InstCombine] icmp eq/ne (gep inbounds P, Idx..), null -> icmp eq/ne P, null.
Thu, Aug 22, 11:15 AM · Restricted Project
reames updated the diff for D66608: [InstCombine] icmp eq/ne (gep inbounds P, Idx..), null -> icmp eq/ne P, null.

Fix a typo in one of the tests and add clarifying comments.

Thu, Aug 22, 11:06 AM · Restricted Project
reames created D66608: [InstCombine] icmp eq/ne (gep inbounds P, Idx..), null -> icmp eq/ne P, null.
Thu, Aug 22, 11:06 AM · Restricted Project
reames added a comment to D64533: [IndVars] Special case the problematic (gep inbounds p, iv == nullptr) problem (pr42357) .

See https://reviews.llvm.org/D66608 for an alternate approach in InstCombine as suggested in review here.

Thu, Aug 22, 11:06 AM

Wed, Aug 21

reames updated the diff for D64451: [PoisonChecking] Validate inbounds annotation on getelementptr where possible.

Address the out of bounds base point brought up in review.

Wed, Aug 21, 1:41 PM · Restricted Project
reames added a comment to D64451: [PoisonChecking] Validate inbounds annotation on getelementptr where possible.

Address comments in advance of patch refresh.

Wed, Aug 21, 1:21 PM · Restricted Project
reames committed rG3c4614ff10e2: Add a couple of extra test noticed in post-commit discussion of rL369541 (authored by reames).
Add a couple of extra test noticed in post-commit discussion of rL369541
Wed, Aug 21, 10:00 AM
reames committed rL369546: Add a couple of extra test noticed in post-commit discussion of rL369541.
Add a couple of extra test noticed in post-commit discussion of rL369541
Wed, Aug 21, 9:57 AM
reames committed rG764b0fd5a371: [instcombine] icmp eq/ne (sub C, Y), C -> icmp eq/ne Y, 0 (authored by reames).
[instcombine] icmp eq/ne (sub C, Y), C -> icmp eq/ne Y, 0
Wed, Aug 21, 8:54 AM
reames committed rL369541: [instcombine] icmp eq/ne (sub C, Y), C -> icmp eq/ne Y, 0.
[instcombine] icmp eq/ne (sub C, Y), C -> icmp eq/ne Y, 0
Wed, Aug 21, 8:54 AM

Mon, Aug 19

reames created D66450: [POC Only] Demonstration of approach to undo exit value rewriting.
Mon, Aug 19, 3:59 PM
reames accepted D64869: [SCEV] get more accurate range for AddExpr with NW flag.

LGTM

Mon, Aug 19, 1:59 PM · Restricted Project

Fri, Aug 16

reames planned changes to D65354: [X86] Let MachineCombiner reassociate adds for ILP.

Marking as "Plan Changes" to cleanup phab views.

Fri, Aug 16, 9:45 AM · Restricted Project
reames planned changes to D65614: [Reassociate] Stop linearizing all associative expression trees w/o profitability.

Marking as "Plan Changes" to cleanup phab views.

Fri, Aug 16, 9:45 AM · Restricted Project

Thu, Aug 15

reames updated the summary of D66309: Introduce infrastructure for an incremental port of SelectionDAG atomic load/store handling.
Thu, Aug 15, 4:50 PM · Restricted Project
reames created D66322: [X86] Updated target specific selection dag code to conservatively check for isAtomic in addition to isVolatile.
Thu, Aug 15, 4:45 PM · Restricted Project
reames updated the diff for D66318: [SDAG] Update generic code to conservatively check for isAtomic in addition to isVolatile.

update all todos with an easily grepable tag for ease of future work

Thu, Aug 15, 4:15 PM · Restricted Project
reames created D66318: [SDAG] Update generic code to conservatively check for isAtomic in addition to isVolatile.
Thu, Aug 15, 4:09 PM · Restricted Project
reames updated the summary of D66309: Introduce infrastructure for an incremental port of SelectionDAG atomic load/store handling.
Thu, Aug 15, 4:00 PM · Restricted Project
reames committed rG5c38ca35346f: [SDAG] Minor code cleanup/standardization of atomic accessors [NFC] (authored by reames).
[SDAG] Minor code cleanup/standardization of atomic accessors [NFC]
Thu, Aug 15, 3:25 PM
reames committed rL369057: [SDAG] Minor code cleanup/standardization of atomic accessors [NFC].
[SDAG] Minor code cleanup/standardization of atomic accessors [NFC]
Thu, Aug 15, 3:25 PM
reames created D66309: Introduce infrastructure for an incremental port of SelectionDAG atomic load/store handling.
Thu, Aug 15, 1:03 PM · Restricted Project
reames committed rGd202899431e4: [NFC] Add a couple of dump routines for RegisterPressure helper classes (authored by reames).
[NFC] Add a couple of dump routines for RegisterPressure helper classes
Thu, Aug 15, 11:52 AM
reames committed rL369037: [NFC] Add a couple of dump routines for RegisterPressure helper classes.
[NFC] Add a couple of dump routines for RegisterPressure helper classes
Thu, Aug 15, 11:52 AM

Wed, Aug 14

reames committed rG7b0515176b15: [SCEV] Rename getMaxBackedgeTakenCount to getConstantMaxBackedgeTakenCount [NFC] (authored by reames).
[SCEV] Rename getMaxBackedgeTakenCount to getConstantMaxBackedgeTakenCount [NFC]
Wed, Aug 14, 3:00 PM
reames committed rL368930: [SCEV] Rename getMaxBackedgeTakenCount to getConstantMaxBackedgeTakenCount [NFC].
[SCEV] Rename getMaxBackedgeTakenCount to getConstantMaxBackedgeTakenCount [NFC]
Wed, Aug 14, 3:00 PM
reames created D66242: [POC][SCEV] Bounds for loop invariant exits.
Wed, Aug 14, 12:49 PM · Restricted Project
reames committed rG6cca3ad43e6d: [RLEV] Rewrite loop exit values for multiple exit loops w/o overall loop exit… (authored by reames).
[RLEV] Rewrite loop exit values for multiple exit loops w/o overall loop exit…
Wed, Aug 14, 11:28 AM
reames committed rL368898: [RLEV] Rewrite loop exit values for multiple exit loops w/o overall loop exit….
[RLEV] Rewrite loop exit values for multiple exit loops w/o overall loop exit…
Wed, Aug 14, 11:27 AM
reames closed D65544: [RLEV] Rewrite loop exit values for multiple exit loops w/o overall loop exit count.
Wed, Aug 14, 11:27 AM · Restricted Project
reames added a comment to D66161: [SLC] Dereferenceable annonation - handle valid null pointers.

I have no problem with this patch (i.e. it can land if Johannes approves)

Wed, Aug 14, 9:46 AM · Restricted Project

Tue, Aug 13

reames added a comment to D66161: [SLC] Dereferenceable annonation - handle valid null pointers.

...
Johannes, I didn't take time to fully understand your concern, but if it's broader than the above, please revert the original patch and handle in normal review.

Hey, read back over this and realized my tone rather snappy here. Sorry about that, was annoyed at something else at the time I wrote this and it came through more than I realized. Sorry.

Tue, Aug 13, 3:04 PM · Restricted Project
reames added a comment to D66161: [SLC] Dereferenceable annonation - handle valid null pointers.

I'm fine with this, @reames ?

Definitely not. I've never seen that utility on Function, but whatever it's supposed to do, it completely ignores address spaces (our major source of defined nulls) while doing it.

Tue, Aug 13, 2:37 PM · Restricted Project

Fri, Aug 9

reames added a comment to D65718: [LangRef] Document forward-progress requirement.

Just a note of caution: I see discussion starting on cornercases. I would strongly suggest *not* blocking this patch on getting a fully consistent definition. Having something documented which is "mostly right" is better than no documentation at all. (Adding a explicit marker of subtleties under discussion is good.) It's really tempting as reviewers to keep looking for the subtle problems, but remember, the confusion which keeps arising is over the basic need for a side effect, not the exact definition thereof!

Fri, Aug 9, 8:35 PM · Restricted Project

Tue, Aug 6

reames requested changes to D64869: [SCEV] get more accurate range for AddExpr with NW flag.
Tue, Aug 6, 1:29 PM · Restricted Project

Mon, Aug 5

reames committed rGe39e79358fcd: Add a note to the release not about a potentially breaking optimization (authored by reames).
Add a note to the release not about a potentially breaking optimization
Mon, Aug 5, 3:38 PM
reames committed rL367941: Add a note to the release not about a potentially breaking optimization.
Add a note to the release not about a potentially breaking optimization
Mon, Aug 5, 3:38 PM
reames accepted D65718: [LangRef] Document forward-progress requirement.

LGTM w/suggested minor changes.

Mon, Aug 5, 3:08 PM · Restricted Project
reames committed rG9bf59384c640: Robustify update_test_checks.py to non-autogened tests, and add a mode to skip… (authored by reames).
Robustify update_test_checks.py to non-autogened tests, and add a mode to skip…
Mon, Aug 5, 11:26 AM
reames committed rL367900: Robustify update_test_checks.py to non-autogened tests, and add a mode to skip….
Robustify update_test_checks.py to non-autogened tests, and add a mode to skip…
Mon, Aug 5, 11:25 AM
reames closed D65610: Robustify update_test_checks.py to non-autogened tests, and add a mode to skip non-autogenerated ones.
Mon, Aug 5, 11:24 AM · Restricted Project

Fri, Aug 2

reames committed rG511be2a15890: [Statepoints] Fix overalignment of loads in no-realign-stack functions (authored by reames).
[Statepoints] Fix overalignment of loads in no-realign-stack functions
Fri, Aug 2, 1:19 PM
reames committed rL367718: [Statepoints] Fix overalignment of loads in no-realign-stack functions.
[Statepoints] Fix overalignment of loads in no-realign-stack functions
Fri, Aug 2, 1:19 PM
reames committed rG5f8e570b3ce8: [Test] Demonstrate a realignment bug missed in r366765 (authored by reames).
[Test] Demonstrate a realignment bug missed in r366765
Fri, Aug 2, 1:02 PM
reames committed rL367714: [Test] Demonstrate a realignment bug missed in r366765.
[Test] Demonstrate a realignment bug missed in r366765
Fri, Aug 2, 1:01 PM
reames added a comment to D65610: Robustify update_test_checks.py to non-autogened tests, and add a mode to skip non-autogenerated ones.

Could this functionality be added to all the update*py scripts?

Could be reasonable done, but given the divergence already existing, would need to be hand implemented for each. I'm not volunteering.

Fri, Aug 2, 11:02 AM · Restricted Project
reames updated the diff for D65610: Robustify update_test_checks.py to non-autogened tests, and add a mode to skip non-autogenerated ones.
Fri, Aug 2, 11:02 AM · Restricted Project
reames updated the diff for D65614: [Reassociate] Stop linearizing all associative expression trees w/o profitability.

Missed a condition in the POC

Fri, Aug 2, 10:49 AM · Restricted Project
reames updated the diff for D65614: [Reassociate] Stop linearizing all associative expression trees w/o profitability.

POC fix for the CSE pointed out in review. This isn't a "real" patch yet, more of a hint as to a possible direction. I'm fairly sure we *could* make this sufficiently fast if we want to move in this direction by memoizing expression trees.

Fri, Aug 2, 10:48 AM · Restricted Project
reames added a comment to D65614: [Reassociate] Stop linearizing all associative expression trees w/o profitability.

I'm seeing many regressions in diffs.

Can you point to a specific example of what you mean? I haven't studied every single one closely, but on a skim, I am not seeing regressions, so I'm curious what you mean?

Hm? There's clearly a lot of cases where instruction count increases.

Gah, you're right, my sample was bogus. What I get for skimming from the bottom of a large diff, not the top.

Fri, Aug 2, 10:28 AM · Restricted Project

Thu, Aug 1

reames added a comment to D65614: [Reassociate] Stop linearizing all associative expression trees w/o profitability.

I'm seeing many regressions in diffs.

Can you point to a specific example of what you mean? I haven't studied every single one closely, but on a skim, I am not seeing regressions, so I'm curious what you mean?

Thu, Aug 1, 5:03 PM · Restricted Project
reames accepted D64972: [Loop Peeling] Do not close further unroll/peel if profile based peeling was not used.

Serguei and I spent a fair amount of time talking about this one offline. Neither of us are completely happy with the structure of this patch, but since it's using the same pattern as what was already there (just a bit more fine grained), I decide to approve this so as to unblock Serguei's other work.

Thu, Aug 1, 5:03 PM · Restricted Project
reames updated the diff for D65614: [Reassociate] Stop linearizing all associative expression trees w/o profitability.

Rebase remainder of test changes after autogening them in a separate commit..

Thu, Aug 1, 4:34 PM · Restricted Project
reames committed rG2264f96c2a3b: [Tests] Autogen a bunch of Reassociate tests for ease of update (authored by reames).
[Tests] Autogen a bunch of Reassociate tests for ease of update
Thu, Aug 1, 4:32 PM
reames committed rL367634: [Tests] Autogen a bunch of Reassociate tests for ease of update.
[Tests] Autogen a bunch of Reassociate tests for ease of update
Thu, Aug 1, 4:29 PM
reames added a comment to D65354: [X86] Let MachineCombiner reassociate adds for ILP.

Skimming through the RegisterPressure class and the approach MachineLICM uses, the mechanics of adding regrester pressure tracking to MachineCombiner don't seem too bad. I'm fairly confident we can do so without too much work, though I haven't fully worked it through much less written the code, so that might be a gotcha. Thanks everyone for the pointers.

Thu, Aug 1, 4:19 PM · Restricted Project
reames created D65614: [Reassociate] Stop linearizing all associative expression trees w/o profitability.
Thu, Aug 1, 3:34 PM · Restricted Project
reames added a comment to D65354: [X86] Let MachineCombiner reassociate adds for ILP.

I've posted an alternate fix for my actual problem over in https://reviews.llvm.org/D65614. It turns out the linear schedule which MachineCombiner would be undoing here is entirely self inflicted.

Thu, Aug 1, 3:34 PM · Restricted Project
reames created D65610: Robustify update_test_checks.py to non-autogened tests, and add a mode to skip non-autogenerated ones.
Thu, Aug 1, 3:13 PM · Restricted Project

Wed, Jul 31

reames committed rG79c27c9464f1: Fix a release-only build warning triggered by rL367485 (authored by reames).
Fix a release-only build warning triggered by rL367485
Wed, Jul 31, 6:18 PM
reames added inline comments to rL367485: [IndVars, RLEV] Support rewriting exit values in loops without known exits….
Wed, Jul 31, 6:18 PM
reames committed rL367499: Fix a release-only build warning triggered by rL367485.
Fix a release-only build warning triggered by rL367485
Wed, Jul 31, 6:15 PM
reames committed rGc724215a7003: Attempt to unbreak sphinx build bot by inserting a link. (authored by reames).
Attempt to unbreak sphinx build bot by inserting a link.
Wed, Jul 31, 3:20 PM
reames added a comment to D65164: Define some basic terminology around loops in our documentation.

This broke the sphinx bot:
...
Can you add a reference to this somewhere?

Speculative fix in revision 367487.

Wed, Jul 31, 3:20 PM · Restricted Project
reames committed rL367487: Attempt to unbreak sphinx build bot by inserting a link..
Attempt to unbreak sphinx build bot by inserting a link.
Wed, Jul 31, 3:14 PM
reames created D65544: [RLEV] Rewrite loop exit values for multiple exit loops w/o overall loop exit count.
Wed, Jul 31, 3:03 PM · Restricted Project
reames committed rGf8e7b536571e: [IndVars, RLEV] Support rewriting exit values in loops without known exits… (authored by reames).
[IndVars, RLEV] Support rewriting exit values in loops without known exits…
Wed, Jul 31, 2:17 PM
reames committed rL367485: [IndVars, RLEV] Support rewriting exit values in loops without known exits….
[IndVars, RLEV] Support rewriting exit values in loops without known exits…
Wed, Jul 31, 2:17 PM
reames added a comment to D65164: Define some basic terminology around loops in our documentation.

I am unhappy about this having been committed already. Now we have official documentation at https://llvm.org/docs/LoopTerminology.html which is partially wrong.

In retrospect, I probably landed this a bit quickly. I interpreted the comments as incremental refinement, and missed the one about cycles vs SCCs being correctness related. I believe the sole correctness issue has been resolved. If you see any others, please point them out.

Wed, Jul 31, 10:10 AM · Restricted Project
reames committed rGf3b752365e69: [docs] Reword documentation in terms of SCCs not cycles (authored by reames).
[docs] Reword documentation in terms of SCCs not cycles
Wed, Jul 31, 9:25 AM
reames committed rL367440: [docs] Reword documentation in terms of SCCs not cycles.
[docs] Reword documentation in terms of SCCs not cycles
Wed, Jul 31, 9:23 AM
reames closed D65299: Try to clarify loop definition around confusing sub-loop case.
Wed, Jul 31, 9:23 AM · Restricted Project

Tue, Jul 30

reames requested changes to D64972: [Loop Peeling] Do not close further unroll/peel if profile based peeling was not used.

Having both the enable flag and the AlreadyPeeled variable is really confusing. Is there a way we could combine them? Maybe replace the boolean with the AlreadyPeeledViaProfiling count or something?

Tue, Jul 30, 11:53 AM · Restricted Project
reames updated the diff for D65299: Try to clarify loop definition around confusing sub-loop case.

I think I've reformulated this to incorporate the review comments, let me know what you think.

Tue, Jul 30, 11:44 AM · Restricted Project
reames added a comment to D64898: Standardize how we check for legality of frame realignment.

ping

Tue, Jul 30, 11:44 AM · Restricted Project
reames added a comment to D65354: [X86] Let MachineCombiner reassociate adds for ILP.

IIRC, MachineCombiner has the potential to cause spills (it runs without real knowledge of register pressure)...and if we spill, then we have likely killed all hope for a perf improvement due to better throughput of math/logic. This is true for any reassociable opcode, but x86 scalar 'add' has more potential chance for trouble due to prevalence and so few available registers ( (as seen by the number of diffs here?).

So it's correct that there was no principled reason to leave 'add' out, but the practical concern was spilling. I have not looked at the test diffs here, but if there's evidence of extra instructions, then we're probably going to see perf regressions rather than improvements?

I hear the concern here, but not reassociating on adds also means we miss some semi major performance opportunities. Any suggestions on how to break the impasse with a tractable amount of work? As in, any suggestions on how to implement a reasonable register pressure heuristic in MachineCombiner?

Tue, Jul 30, 11:30 AM · Restricted Project
reames planned changes to D64533: [IndVars] Special case the problematic (gep inbounds p, iv == nullptr) problem (pr42357) .

Just indicating in the review state that the next action item here is mine. Probably not going to get back to this for a bit, so want that to be clear to reviewers.

Tue, Jul 30, 11:26 AM
reames planned changes to D64451: [PoisonChecking] Validate inbounds annotation on getelementptr where possible.

Just indicating in the review state that the next action item here is mine. Probably not going to get back to this for a bit, so want that to be clear to reviewers.

Tue, Jul 30, 11:25 AM · Restricted Project
reames planned changes to D64509: [SCEV] Compute exit count from overflow test.

Just indicating in the review state that the next action item here is mine. Probably not going to get back to this for a bit, so want that to be clear to reviewers.

Tue, Jul 30, 11:25 AM · Restricted Project

Fri, Jul 26

reames created D65354: [X86] Let MachineCombiner reassociate adds for ILP.
Fri, Jul 26, 3:50 PM · Restricted Project

Jul 25 2019

reames planned changes to D65257: Describe resticted form of loops in the new loop terminogy documentation.
Jul 25 2019, 2:21 PM · Restricted Project
reames closed D65164: Define some basic terminology around loops in our documentation.
Jul 25 2019, 2:17 PM · Restricted Project
reames accepted D65164: Define some basic terminology around loops in our documentation.

Reaccept a review so that I can re-close, as the patch was submitted.

Jul 25 2019, 2:16 PM · Restricted Project
reames added a comment to D65164: Define some basic terminology around loops in our documentation.

An attempt at fixing the wording around the ambiguity just discussed is here: https://reviews.llvm.org/D65299

Jul 25 2019, 2:16 PM · Restricted Project
reames created D65299: Try to clarify loop definition around confusing sub-loop case.
Jul 25 2019, 2:16 PM · Restricted Project