reames (Philip Reames)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Wed, Sep 19

reames requested changes to D44748: Track whether the size of a MemoryLocation is precise.

Mostly minor comments. I considered giving a conditional LGTM, but decided it warranted one last round. I think this is very close to landing.

Wed, Sep 19, 3:58 PM

Mon, Sep 17

reames added a comment to D52192: Add !thread.private metadata .

Just noting a couple of known gaps (to save reviewers time):

  • No verifier support (will add before submission)
  • Need to add references from each support instruction type in the docs (done to avoid rebase headache)
Mon, Sep 17, 2:37 PM
reames retitled D52192: Add !thread.private metadata from Add !thread.private metdata to Add !thread.private metadata .
Mon, Sep 17, 2:30 PM
reames created D52192: Add !thread.private metadata .
Mon, Sep 17, 2:30 PM

Mon, Sep 10

reames committed rL341892: [LICM] (re-)simplify code using MemoryLocation API [NFC].
[LICM] (re-)simplify code using MemoryLocation API [NFC]
Mon, Sep 10, 8:29 PM
reames created D51907: [WIP][LICM] Hoist argmemonly calls which can write through some arguments.
Mon, Sep 10, 8:16 PM
reames added inline comments to D51895: Fix issues required to remove memset/memcpy/memmove special casing from AliasSetTracker.
Mon, Sep 10, 4:16 PM
reames committed rL341880: [AST] Add test coverage of memsets.
[AST] Add test coverage of memsets
Mon, Sep 10, 4:15 PM
reames created D51895: Fix issues required to remove memset/memcpy/memmove special casing from AliasSetTracker.
Mon, Sep 10, 3:53 PM
reames requested changes to D50377: [MustExecute] Rework LoopSafetyInfo to make it more optimistic about throws.

Another round of bugs found.

Mon, Sep 10, 2:01 PM
reames accepted D51207: Introduce llvm.experimental.widenable_condition intrinsic.

I'm approving this in the current form, despite a bit of hesitation doing so. I'd like to see the conversation around the restriction of widening based on target block to continue. I think there's a good change we'll want to tweak the semantics there, but I see that as a minor tweak, not a major redesign.

Mon, Sep 10, 1:46 PM
reames committed rL341841: [AST] Visit memtransfer arguments in order.
[AST] Visit memtransfer arguments in order
Mon, Sep 10, 9:01 AM

Fri, Sep 7

reames committed rL341713: [AST] Generalize argument specific aliasing.
[AST] Generalize argument specific aliasing
Fri, Sep 7, 2:40 PM
reames closed D50730: [AST] Generalize argument specific aliasing.
Fri, Sep 7, 2:40 PM
reames added inline comments to D50730: [AST] Generalize argument specific aliasing.
Fri, Sep 7, 10:33 AM
reames accepted D51715: [LICM] Avoid duplicate work during building AliasSetTracker.

Looked at the history in more detail, and decided I'm comfortable with this fix. So, LGTM.

Fri, Sep 7, 10:30 AM

Thu, Sep 6

reames added inline comments to D51715: [LICM] Avoid duplicate work during building AliasSetTracker.
Thu, Sep 6, 4:38 PM
reames added a comment to D51715: [LICM] Avoid duplicate work during building AliasSetTracker.

This looks correct to me, but this is so glaringly bad I'd like a second set of eyes to make sure I'm not missing something. Can someone else confirm?

Thu, Sep 6, 4:30 PM

Wed, Sep 5

reames requested changes to D51207: Introduce llvm.experimental.widenable_condition intrinsic.

Overall, looks pretty good. One rounds of comments, but I'm expecting this to converge quickly.

Wed, Sep 5, 7:19 PM
reames added a comment to D51523: Return "[NFC] Add severe validation of InstructionPrecedenceTracking".

LGTM w/ changes made before submit.

Wed, Sep 5, 12:36 PM

Thu, Aug 30

reames requested changes to D51523: Return "[NFC] Add severe validation of InstructionPrecedenceTracking".
Thu, Aug 30, 9:38 PM

Wed, Aug 29

reames updated the diff for D51471: [SimplifyCFG] Reduce memory churn during common case for branch commoning [NFC].

reverse last rebase, the small addition was buggy (iterator invalidation)

Wed, Aug 29, 5:45 PM
reames updated the diff for D51471: [SimplifyCFG] Reduce memory churn during common case for branch commoning [NFC].

(slightly modified patch to reduce overhead for multiple predecessor case too)

Wed, Aug 29, 5:41 PM
reames created D51471: [SimplifyCFG] Reduce memory churn during common case for branch commoning [NFC].
Wed, Aug 29, 5:34 PM
reames committed rL341004: [SimplifyCFG] Rename a variable for readibility of a future change [NFC].
[SimplifyCFG] Rename a variable for readibility of a future change [NFC]
Wed, Aug 29, 5:13 PM
reames committed rL341001: [SimplifyCFG] Fix a cost modeling oversight in branch commoning.
[SimplifyCFG] Fix a cost modeling oversight in branch commoning
Wed, Aug 29, 5:06 PM
reames committed rL340997: [SimplifyCFG] Common debug handling [NFC].
[SimplifyCFG] Common debug handling [NFC]
Wed, Aug 29, 4:23 PM
reames updated the diff for D50730: [AST] Generalize argument specific aliasing.

rebase for minor issue I commented on, also ping?

Wed, Aug 29, 3:39 PM
reames committed rL340978: Add a todo and tests to Address a review commnt from D50925 [NFC].
Add a todo and tests to Address a review commnt from D50925 [NFC]
Wed, Aug 29, 3:11 PM
reames added inline comments to D50925: [LICM] Hoist stores of invariant values to invariant addresses out of loops.
Wed, Aug 29, 3:11 PM
reames committed rL340974: [LICM] Hoist stores of invariant values to invariant addresses out of loops.
[LICM] Hoist stores of invariant values to invariant addresses out of loops
Wed, Aug 29, 2:50 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Wed, Aug 29, 2:50 PM
reames created D51458: [WIP] Support merging common destinations checks w/uses outside of block.
Wed, Aug 29, 2:01 PM

Aug 24 2018

reames abandoned D19559: [LVI] Exploit trivial range information from unknown RHS of icmp.

No longer needed, and extremely stale.

Aug 24 2018, 3:04 PM
reames committed rL340660: [CVP] Extend tests to illustrate an old patch isn't needed.
[CVP] Extend tests to illustrate an old patch isn't needed
Aug 24 2018, 2:57 PM
reames abandoned D7061: Allow PRE to duplicate loads in LICM like loop case.
Aug 24 2018, 2:49 PM
reames abandoned D14263: [LVI] Clarify invariants, common code, and fix latent bugs.
Aug 24 2018, 2:48 PM
reames committed rL340638: [AST] Simplify code minorly using pattern match [NFC].
[AST] Simplify code minorly using pattern match [NFC]
Aug 24 2018, 12:14 PM
reames added inline comments to D50730: [AST] Generalize argument specific aliasing.
Aug 24 2018, 9:34 AM
reames committed rL340617: [LICM] Hoist an invariant_start out of loops if there are no stores executed….
[LICM] Hoist an invariant_start out of loops if there are no stores executed…
Aug 24 2018, 9:25 AM
reames closed D51181: [LICM] Hoist an invariant_start out of loops if there are no stores executed before it.
Aug 24 2018, 9:25 AM
reames abandoned D46212: [WIP][LICM] Hoisting of guards, assumes, and invariant_starts.

All component pieces have been split and landed individually.

Aug 24 2018, 9:18 AM
reames updated the summary of D46212: [WIP][LICM] Hoisting of guards, assumes, and invariant_starts.
Aug 24 2018, 9:16 AM

Aug 23 2018

reames created D51181: [LICM] Hoist an invariant_start out of loops if there are no stores executed before it.
Aug 23 2018, 1:54 PM
reames added a comment to D50925: [LICM] Hoist stores of invariant values to invariant addresses out of loops.

Added example:

(snipped example for length)

Aug 23 2018, 1:03 PM
reames added a comment to D50925: [LICM] Hoist stores of invariant values to invariant addresses out of loops.

There are 3 kinds of tests worth adding:

  1. predicated invariant stores, i.e. the block containing the store itself is predicated and not guaranteed to execute (cannot be handled by LICM)

Covered by existing early exit test.

  1. invariant store value is a phi containing invariant incoming values and the phi result depends on an invariant condition (can be handled by LICM. This patch handles?)

Unclear what you mean here.

  1. invariant store value is a phi containing invariant incoming values and the phi result depends on a variant condition (cannot be handled by LICM safely)

Again, I don't know what you mean by this. If the value is a phi in the loop, it's by definition not invariant.

Aug 23 2018, 12:19 PM
reames updated the diff for D50730: [AST] Generalize argument specific aliasing.

with the right patch this time

Aug 23 2018, 12:14 PM
reames updated the diff for D50730: [AST] Generalize argument specific aliasing.

rebase and incorporate suggestions.

Aug 23 2018, 12:10 PM

Aug 22 2018

reames committed rL340453: [AST] Add a test for attribute intersection.
[AST] Add a test for attribute intersection
Aug 22 2018, 2:11 PM
reames committed rL340443: [AA] Remove a needless variable [NFC].
[AA] Remove a needless variable [NFC]
Aug 22 2018, 12:51 PM
reames committed rL340440: [AST] Minor whitespace cleanup [NFC].
[AST] Minor whitespace cleanup [NFC]
Aug 22 2018, 12:31 PM
reames added inline comments to D50888: [NFC][LICM] Remove too conservative IsMustExecute variable.
Aug 22 2018, 12:28 PM
reames updated the diff for D50925: [LICM] Hoist stores of invariant values to invariant addresses out of loops.

Adding requested tests, ready for review.

Aug 22 2018, 9:21 AM

Aug 21 2018

reames committed rL340384: [AST] Fix a whitespace typo [NFC].
[AST] Fix a whitespace typo [NFC]
Aug 21 2018, 8:37 PM
reames committed rL340383: [AST] Reorder code to reduce a future patch diff [NFC].
[AST] Reorder code to reduce a future patch diff [NFC]
Aug 21 2018, 8:34 PM
reames committed rL340382: [AST] Move a function definition into the cpp [NFC].
[AST] Move a function definition into the cpp [NFC]
Aug 21 2018, 8:33 PM
reames planned changes to D50925: [LICM] Hoist stores of invariant values to invariant addresses out of loops.
Aug 21 2018, 8:30 PM
reames updated the diff for D50925: [LICM] Hoist stores of invariant values to invariant addresses out of loops.

Address first couple comments, another update to come before re-review justified.

Aug 21 2018, 8:30 PM
reames planned changes to D50925: [LICM] Hoist stores of invariant values to invariant addresses out of loops.
Aug 21 2018, 8:21 PM
reames updated the summary of D50925: [LICM] Hoist stores of invariant values to invariant addresses out of loops.
Aug 21 2018, 2:45 PM
reames retitled D50925: [LICM] Hoist stores of invariant values to invariant addresses out of loops from [WIP] [LICM] Hoist stores of invariant values to invariant addresses out of loops to [LICM] Hoist stores of invariant values to invariant addresses out of loops.
Aug 21 2018, 2:44 PM
reames updated the diff for D50925: [LICM] Hoist stores of invariant values to invariant addresses out of loops.

Strip out the call hoisting bits until a future patch.

Aug 21 2018, 2:43 PM
reames updated the diff for D50925: [LICM] Hoist stores of invariant values to invariant addresses out of loops.

Rebase on top of tests, with test updates.

Aug 21 2018, 2:40 PM
reames committed rL340344: [LICM] Fix a test so it actualy checks what was meant [NFC].
[LICM] Fix a test so it actualy checks what was meant [NFC]
Aug 21 2018, 2:28 PM
reames closed D50786: [AST] Remove notion of volatile from alias sets.

Committed as:
Author: reames
Date: Tue Aug 21 10:59:11 2018
New Revision: 340312

Aug 21 2018, 11:38 AM
reames committed rL340312: [AST] Remove notion of volatile from alias sets [NFCI].
[AST] Remove notion of volatile from alias sets [NFCI]
Aug 21 2018, 11:00 AM

Aug 20 2018

reames requested changes to D50377: [MustExecute] Rework LoopSafetyInfo to make it more optimistic about throws.
Aug 20 2018, 10:15 PM
reames committed rL340245: [AST] Mark invariant.starts as being readonly.
[AST] Mark invariant.starts as being readonly
Aug 20 2018, 5:56 PM
reames closed D50861: [AST] Mark invariant.starts as being readonly.
Aug 20 2018, 5:56 PM
reames updated the diff for D50786: [AST] Remove notion of volatile from alias sets.

Max, I landed the changes for the test separately. I applied your check-label request, but not one about single iteration loops since the test file is filed with such. Anyone who wants to specialize single iteration loops has some work to do later anyways, so I didn't see any value.

Aug 20 2018, 5:45 PM
reames committed rL340244: [LICM] Add tests from D50786 [NFC].
[LICM] Add tests from D50786 [NFC]
Aug 20 2018, 5:43 PM
reames committed rL340243: [LICM][NFC] Add tests from D50730.
[LICM][NFC] Add tests from D50730
Aug 20 2018, 5:37 PM
reames committed rL340242: [LICM] More tests for D50925 [NFC].
[LICM] More tests for D50925 [NFC]
Aug 20 2018, 5:15 PM
reames committed rL340233: [LICM][Tests] Add tests for store hoisting [NFC].
[LICM][Tests] Add tests for store hoisting [NFC]
Aug 20 2018, 4:38 PM

Aug 17 2018

reames committed rL340108: [AST] Clarify printing of unknown size locations [NFC].
[AST] Clarify printing of unknown size locations [NFC]
Aug 17 2018, 4:18 PM
reames added inline comments to D50730: [AST] Generalize argument specific aliasing.
Aug 17 2018, 3:08 PM
reames committed rL340100: [AST][Tests] Clarify what each test is doing.
[AST][Tests] Clarify what each test is doing
Aug 17 2018, 2:59 PM
reames committed rL340099: [AST[Tests] Shorten tests using noalias params.
[AST[Tests] Shorten tests using noalias params
Aug 17 2018, 2:46 PM
reames committed rL340095: [AST] Add tests for argmemonly calls [NFC].
[AST] Add tests for argmemonly calls [NFC]
Aug 17 2018, 2:43 PM
reames created D50925: [LICM] Hoist stores of invariant values to invariant addresses out of loops.
Aug 17 2018, 2:12 PM
reames added a comment to D50838: [NFC] Remove function isGuaranteedToExecute.

Hold this one please. I have some concerns about the naming, but have lost track of the dependent patches and can't fully review. Once dependent changes have landed, let's revisit.

Aug 17 2018, 2:01 PM
reames accepted D50890: [NFC] Factor out predecessors collection into a separate method.

LGTM. Don't see where you're going with this, but the patch is obvious correct and doesn't make anything particularly messy.

Aug 17 2018, 1:56 PM

Aug 16 2018

reames accepted D50854: [LICM] Add a diagnostic analysis for identifying alias information.

I still feel the framing comment could use some improvement, but I don't have anything specific I can suggest.

Aug 16 2018, 2:25 PM
reames committed rL339937: [AST] Speculative build fix for a polly buildbot.
[AST] Speculative build fix for a polly buildbot
Aug 16 2018, 1:59 PM
reames added inline comments to D50861: [AST] Mark invariant.starts as being readonly.
Aug 16 2018, 1:50 PM
reames committed rL339936: [MemLoc] Fix a bug causing any use of invariant.end to crash in LICM.
[MemLoc] Fix a bug causing any use of invariant.end to crash in LICM
Aug 16 2018, 1:49 PM
reames created D50861: [AST] Mark invariant.starts as being readonly.
Aug 16 2018, 1:29 PM
reames committed rL339930: [LICM][NFC] Restructure pointer invalidation API in terms of MemoryLocation.
[LICM][NFC] Restructure pointer invalidation API in terms of MemoryLocation
Aug 16 2018, 1:12 PM
reames updated the summary of D46212: [WIP][LICM] Hoisting of guards, assumes, and invariant_starts.
Aug 16 2018, 12:10 PM
reames requested changes to D50854: [LICM] Add a diagnostic analysis for identifying alias information.

Detailed comments inline.

Aug 16 2018, 11:14 AM
reames accepted D50558: [MustExecute] Fix algorithmic bug in isGuaranteedToExecute. PR38514.
Aug 16 2018, 10:55 AM
reames added a comment to D50558: [MustExecute] Fix algorithmic bug in isGuaranteedToExecute. PR38514.

Thinking about it, I think it may be worth introducing a separate isGuaranteedToExecuteBeforeLoopExit with the old logic. While LICM's hoisting logic needs the "must execute if loop entered" property, load store promotion actually only needs "store executes before any taken exits" property. (We do need one "must execute" dereferencing instruction, but it doesn't have to be the store.)

Aug 16 2018, 10:55 AM

Aug 15 2018

reames added a comment to D50480: [LV] Vectorizing loops of arbitrary trip count without remainder under opt for size.

I have a general question about direction, not specific to this patch.

It seems like we're adding a specific form of predication to the vectorizer in this patch and I know we already have support for various predicated load and store idioms. What are our plans in terms of supporting more general predication? For instance, I don't believe we handle loops like the following at the moment:
for (int i = 0; i < N; i++) {

if (unlikely(i > M)) 
   break;
sum += a[i];

}

Can the infrastructure in this patch be generalized to handle such cases? And if so, are their any specific plans to do so?

Short answer is No.

From vectorizer perspective, mechanics is quite different.

Ok, I think we're talking past each other a bit. I see these both as forms of predication. It sounds like you have a slightly different view; I'll try to ask clarifying questions in the right spots. I think we have different mental models here and I'm trying to understand where that difference is.

Aug 15 2018, 11:44 AM
reames added a comment to D50786: [AST] Remove notion of volatile from alias sets.

There was discussion sometime back, might be worth looking at: https://groups.google.com/forum/#!topic/llvm-dev/uLvitfqEd_g

I'm missing why this is relevant? It happens to involve a volatile access as an AliasSet, but seems otherwise unrelated. From what I can tell, the "volatile implies mod" thing hasn't been true for years.

Aug 15 2018, 11:08 AM
reames requested changes to D50558: [MustExecute] Fix algorithmic bug in isGuaranteedToExecute. PR38514.
Aug 15 2018, 9:53 AM
reames added a comment to D50558: [MustExecute] Fix algorithmic bug in isGuaranteedToExecute. PR38514.

Max convinced me that there's no algorithmic complexity change implied here. Given the getLoopExits code walks all edges leaving blocks in the loop, both the old and new code is O(outbound edges of blocks in loop).

Aug 15 2018, 9:53 AM
reames created D50786: [AST] Remove notion of volatile from alias sets.
Aug 15 2018, 9:27 AM

Aug 14 2018

reames accepted D50693: [NFC] Refactoring of LoopSafetyInfo, step 1.

LGTM

Aug 14 2018, 9:09 PM
reames added a comment to D48602: `llvm.experimental.stackmap` is erroneously marked `Throws`?.

Do you know if anyone is actually using the stackmap functionality? As far as I know JavaScriptCore used to use it, but have since moved on.

Not that I know of. We (Azul) are using the statepoint infrastructure which shares many of the same ideas, but we are not using either STACKMAP or PATCHPOINT. I suspect we could generalize if needed, patches and discussion welcome!

Aug 14 2018, 3:35 PM
reames added inline comments to D50480: [LV] Vectorizing loops of arbitrary trip count without remainder under opt for size.
Aug 14 2018, 3:25 PM