Page MenuHomePhabricator

reames (Philip Reames)
User

Projects

User does not belong to any projects.

User Details

User Since
Oct 23 2013, 6:32 PM (287 w, 33 m)

Recent Activity

Today

reames added a comment to D61075: [CodeGenPrepare] delay instruction deletion for efficiency.

I think I'm missing something basic here. CGP will iterate until done, including trivial DCE. Given that, why not just leave the instructions in the IR and let the next iteration remove them? It would trigger an extra iteration, which might be undesirable, but would it be incorrect? That would seem better than invalidating the DomTree, which just forces a new iteration anyways...

Wed, Apr 24, 6:55 PM · Restricted Project
reames committed rG7c8647b26f05: [InstCombine] Be consistent w/handling of masked intrinsics style wise [NFC] (authored by reames).
[InstCombine] Be consistent w/handling of masked intrinsics style wise [NFC]
Wed, Apr 24, 6:18 PM
reames committed rL359160: [InstCombine] Be consistent w/handling of masked intrinsics style wise [NFC].
[InstCombine] Be consistent w/handling of masked intrinsics style wise [NFC]
Wed, Apr 24, 6:18 PM

Yesterday

reames committed rG2ce017026af5: [InstCombine] Convert a masked.load of a dereferenceable address to an… (authored by reames).
[InstCombine] Convert a masked.load of a dereferenceable address to an…
Tue, Apr 23, 8:25 AM
reames committed rL359000: [InstCombine] Convert a masked.load of a dereferenceable address to an….
[InstCombine] Convert a masked.load of a dereferenceable address to an…
Tue, Apr 23, 8:25 AM
reames closed D59703: Convert a masked.load of a dereferenceable address to an unconditional load.
Tue, Apr 23, 8:25 AM · Restricted Project

Mon, Apr 22

reames requested changes to D60769: [SimpleLoopUnswitch] Discard stale prof branch_weights for partial unswitched branches.

The change description and your tests make it hard to determine what your intended result is. Please fix.

Mon, Apr 22, 3:05 PM · Restricted Project
reames committed rGd748689c7f71: [InstCombine] Eliminate stores to constant memory (authored by reames).
[InstCombine] Eliminate stores to constant memory
Mon, Apr 22, 1:27 PM
reames committed rL358919: [InstCombine] Eliminate stores to constant memory.
[InstCombine] Eliminate stores to constant memory
Mon, Apr 22, 1:26 PM
reames closed D60659: [InstCombine] Eliminate stores to constant memory.
Mon, Apr 22, 1:26 PM · Restricted Project
reames added inline comments to D60975: Convert a masked.gather of at most one element to a masked.load.
Mon, Apr 22, 12:29 PM · Restricted Project
reames committed rGd8d9b7b20e7b: [InstSimplify] Move masked.gather w/no active lanes handling to InstSimplify… (authored by reames).
[InstSimplify] Move masked.gather w/no active lanes handling to InstSimplify…
Mon, Apr 22, 12:29 PM
reames committed rL358913: [InstSimplify] Move masked.gather w/no active lanes handling to InstSimplify….
[InstSimplify] Move masked.gather w/no active lanes handling to InstSimplify…
Mon, Apr 22, 12:27 PM
reames added a comment to D59703: Convert a masked.load of a dereferenceable address to an unconditional load.

The masked.gather part is split to D60975.

Mon, Apr 22, 11:58 AM · Restricted Project
reames created D60975: Convert a masked.gather of at most one element to a masked.load.
Mon, Apr 22, 11:57 AM · Restricted Project
reames committed rGf01583d09751: [Tests] Revise a test as requested by reviewer in D59703 (authored by reames).
[Tests] Revise a test as requested by reviewer in D59703
Mon, Apr 22, 11:52 AM
reames committed rL358907: [Tests] Revise a test as requested by reviewer in D59703.
[Tests] Revise a test as requested by reviewer in D59703
Mon, Apr 22, 11:52 AM
reames updated the diff for D59703: Convert a masked.load of a dereferenceable address to an unconditional load.

Split the patch. This review is now only the masked.load part.

Mon, Apr 22, 11:32 AM · Restricted Project
reames committed rG8f470890344f: [Tests] Add a negative test for masked.gather part of D59703 (authored by reames).
[Tests] Add a negative test for masked.gather part of D59703
Mon, Apr 22, 11:29 AM
reames committed rL358906: [Tests] Add a negative test for masked.gather part of D59703.
[Tests] Add a negative test for masked.gather part of D59703
Mon, Apr 22, 11:26 AM
reames planned changes to D59703: Convert a masked.load of a dereferenceable address to an unconditional load.

These are 2 independent transforms, right? It would be better to split that into 2 smaller reviews/commits, so we are not missing anything (and easier to diagnose if there's trouble post-commit). The tests should show minimal IR patterns to exercise those 2 independent transforms.

You can view them that way, or not. I was looking at this as "what was needed to decompose a single element gather from a dereferenceable address", but I can see your point. I'll split.

Mon, Apr 22, 10:18 AM · Restricted Project
reames committed rG37104d7189cb: [LPM/BPI] Preserve BPI through trivial loop pass pipeline (e.g. LCSSA… (authored by reames).
[LPM/BPI] Preserve BPI through trivial loop pass pipeline (e.g. LCSSA…
Mon, Apr 22, 10:17 AM
reames added inline comments to D59380: Fold constant & invariant loads into uses over barrier instructions.
Mon, Apr 22, 10:17 AM · Restricted Project
reames committed rL358901: [LPM/BPI] Preserve BPI through trivial loop pass pipeline (e.g. LCSSA….
[LPM/BPI] Preserve BPI through trivial loop pass pipeline (e.g. LCSSA…
Mon, Apr 22, 10:12 AM
reames closed D60790: [LPM/BPI] Preserve BPI through trivial loop pass pipeline.
Mon, Apr 22, 10:12 AM · Restricted Project
reames accepted D60656: [LVI][CVP] Calculate with.overflow result range.

Also, LGTM

Mon, Apr 22, 9:54 AM · Restricted Project
reames accepted D60944: [LSR] Limit the recursion for setup cost.

LGTM

Mon, Apr 22, 9:35 AM · Restricted Project
reames accepted D60636: [CallSite removal] move InlineCost to CallBase usage.

LGTM

Mon, Apr 22, 9:26 AM · Restricted Project

Fri, Apr 19

reames abandoned D57140: [WIP] Teach instcombine how to destroy vector GEPs/loads/stores.

Most of the todos have been checked in, and not an area of current focus. Likely to still post a patch here or there, but they'll be able to stand on their own now that the frameworks in place.

Fri, Apr 19, 12:06 PM
reames added a comment to D58632: [X86] Improve lowering of idemptotent RMW operations.

ping?

Fri, Apr 19, 12:03 PM
reames added a comment to D59380: Fold constant & invariant loads into uses over barrier instructions.

ping?

Fri, Apr 19, 12:03 PM · Restricted Project
reames added a comment to D59703: Convert a masked.load of a dereferenceable address to an unconditional load.

ping?

Fri, Apr 19, 12:03 PM · Restricted Project
reames added a comment to D60659: [InstCombine] Eliminate stores to constant memory.

ping?

Fri, Apr 19, 12:02 PM · Restricted Project
reames planned changes to D60111: [POC] Loop predication w/o guards.

The issue mentioned around needing an exact loop exit count is still present, and after digging into the current code, more fundamental to the implementation than I'd recognized. Will probably return to this, but it's going to be hiatus for the near future.

Fri, Apr 19, 12:01 PM
reames added a comment to D60790: [LPM/BPI] Preserve BPI through trivial loop pass pipeline.

Hal, ping?

Fri, Apr 19, 12:00 PM · Restricted Project

Thu, Apr 18

reames committed rG137995d8daf3: [GuardWidening] Wire up a NPM version of the LoopGuardWidening pass (authored by reames).
[GuardWidening] Wire up a NPM version of the LoopGuardWidening pass
Thu, Apr 18, 12:18 PM
reames committed rL358704: [GuardWidening] Wire up a NPM version of the LoopGuardWidening pass.
[GuardWidening] Wire up a NPM version of the LoopGuardWidening pass
Thu, Apr 18, 12:17 PM
reames committed rGadf288c5d93e: [LoopPred] Fix a blatantly obvious bug in r358684 (authored by reames).
[LoopPred] Fix a blatantly obvious bug in r358684
Thu, Apr 18, 10:04 AM
reames committed rL358688: [LoopPred] Fix a blatantly obvious bug in r358684.
[LoopPred] Fix a blatantly obvious bug in r358684
Thu, Apr 18, 10:04 AM
reames added a comment to D60093: [LoopPredication] Allow predication of loop invariant computations.

Immediately after submitting this, I noticed an important piece had gotten lost somewhere in the rebasing. rL358688 fixes an obvious bug with the invariant_load casing.

Thu, Apr 18, 10:04 AM · Restricted Project
reames committed rG92a7177e6b7f: [LoopPredication] Allow predication of loop invariant computations (within the… (authored by reames).
[LoopPredication] Allow predication of loop invariant computations (within the…
Thu, Apr 18, 9:35 AM
reames committed rL358684: [LoopPredication] Allow predication of loop invariant computations (within the….
[LoopPredication] Allow predication of loop invariant computations (within the…
Thu, Apr 18, 9:35 AM
reames closed D60093: [LoopPredication] Allow predication of loop invariant computations.
Thu, Apr 18, 9:35 AM · Restricted Project
reames committed rGb2c9fc02d526: Fix a bug in SCEV's isSafeToExpand around speculation safety (authored by reames).
Fix a bug in SCEV's isSafeToExpand around speculation safety
Thu, Apr 18, 9:12 AM
reames committed rL358680: Fix a bug in SCEV's isSafeToExpand around speculation safety.
Fix a bug in SCEV's isSafeToExpand around speculation safety
Thu, Apr 18, 9:12 AM
reames added inline comments to D60093: [LoopPredication] Allow predication of loop invariant computations.
Thu, Apr 18, 8:52 AM · Restricted Project
reames added inline comments to D60093: [LoopPredication] Allow predication of loop invariant computations.
Thu, Apr 18, 8:47 AM · Restricted Project
reames updated the diff for D60093: [LoopPredication] Allow predication of loop invariant computations.
Thu, Apr 18, 8:47 AM · Restricted Project
reames updated the diff for D60790: [LPM/BPI] Preserve BPI through trivial loop pass pipeline.

Address review comment

Thu, Apr 18, 8:43 AM · Restricted Project

Wed, Apr 17

reames requested changes to D60600: [InstCombine] Fix a vector-of-pointers instcombine undef bug..

I think you might be overcomplicating this. I'd suggest the following:

  1. scan all index types for a gep
  2. run current code, but if any operand was gep, skip the recursive call on the operand
Wed, Apr 17, 1:59 PM · Restricted Project
reames committed rG88679717ce44: [InstCombine] Factor out unreachable inst idiom creation [NFC] (authored by reames).
[InstCombine] Factor out unreachable inst idiom creation [NFC]
Wed, Apr 17, 10:39 AM
reames committed rL358600: [InstCombine] Factor out unreachable inst idiom creation [NFC].
[InstCombine] Factor out unreachable inst idiom creation [NFC]
Wed, Apr 17, 10:39 AM

Tue, Apr 16

reames added a comment to D60783: [LoopPred] Implement a version of the profitability heuristic w/o BPI.

In the new pass manager, getting access to BPI reliability is a challenge. In practice, we've been running downstream with a variation of this non-BPI based heuristic.

Can you please explain why? The new pass manager was supposed to make our problems getting a hold of analysis results in different places easier, not harder.

I haven't dug through this entirely, but I'm seeing invalidation of BPI being done outside of a loop pass manager. Which is a bit of a problem if I have a loop pass manager which contains two passes, the *first* of which invalidates, and the *second* uses. We appear to be using stale info for the second pass without notice or error.

Tue, Apr 16, 4:06 PM · Restricted Project
reames created D60790: [LPM/BPI] Preserve BPI through trivial loop pass pipeline.
Tue, Apr 16, 12:29 PM · Restricted Project
reames created D60783: [LoopPred] Implement a version of the profitability heuristic w/o BPI.
Tue, Apr 16, 9:52 AM · Restricted Project
reames committed rGc44b68e2b7be: [Tests] Add branch_weights to latches so that test is not effected by future… (authored by reames).
[Tests] Add branch_weights to latches so that test is not effected by future…
Tue, Apr 16, 9:34 AM
reames committed rL358506: [Tests] Add branch_weights to latches so that test is not effected by future….
[Tests] Add branch_weights to latches so that test is not effected by future…
Tue, Apr 16, 9:31 AM

Mon, Apr 15

reames updated the diff for D60093: [LoopPredication] Allow predication of loop invariant computations.

Update after rebasing over previous changes. This is a fairly major rework though, so please review from scratch.

Mon, Apr 15, 1:43 PM · Restricted Project
reames committed rL358439: [Tests] Add a few more tests for LoopPredication w/invariant loads.
[Tests] Add a few more tests for LoopPredication w/invariant loads
Mon, Apr 15, 12:44 PM
reames committed rGaf808ee2ee0e: [Tests] Add a few more tests for LoopPredication w/invariant loads (authored by reames).
[Tests] Add a few more tests for LoopPredication w/invariant loads
Mon, Apr 15, 12:44 PM
reames committed rGe46d77d1d91e: [LoopPred] Stop passing around builders [NFC] (authored by reames).
[LoopPred] Stop passing around builders [NFC]
Mon, Apr 15, 11:14 AM
reames committed rL358434: [LoopPred] Stop passing around builders [NFC].
[LoopPred] Stop passing around builders [NFC]
Mon, Apr 15, 11:14 AM
reames closed D60718: [LoopPred] Stop passing around builders [NFC].
Mon, Apr 15, 11:14 AM · Restricted Project
reames planned changes to D60093: [LoopPredication] Allow predication of loop invariant computations.
Mon, Apr 15, 9:41 AM · Restricted Project
reames added a comment to D60093: [LoopPredication] Allow predication of loop invariant computations.
  1. Since we need to fiddle with the insertion point I think we should stop using single IRBuilder which we pass around. It looks like creating a local IRBuilder is a cheap operation, so we can have local IRBuilders when we need to generate instructions. This way the insertion point will be clear from the local context.

This is handled in a separate review ( D60718 ). Once that's landed, I'll rebase.

Mon, Apr 15, 9:34 AM · Restricted Project
reames created D60718: [LoopPred] Stop passing around builders [NFC].
Mon, Apr 15, 9:33 AM · Restricted Project
reames committed rGfbe64a2cfb41: [LoopPred] Hoist and of predicated checks where legal (authored by reames).
[LoopPred] Hoist and of predicated checks where legal
Mon, Apr 15, 8:53 AM
reames committed rL358419: [LoopPred] Hoist and of predicated checks where legal.
[LoopPred] Hoist and of predicated checks where legal
Mon, Apr 15, 8:53 AM
reames updated the diff for D60093: [LoopPredication] Allow predication of loop invariant computations.

Remove the invariant expression complexity, and the SCEV loop disposition case. I'm still hoping to sink that into SCEV, but handling the simple case is straight forward and I'd like to get at least that in. The SCEV patch turns out to be a bit more complex than I'd originally expected, so I need to take a step back and revaluate that approach.

Mon, Apr 15, 8:25 AM · Restricted Project
reames added a comment to D60659: [InstCombine] Eliminate stores to constant memory.

Why do you think a store through constant would break subtly? For most users I'd expect a store to a constant to correspond with a write to a read-only page causing a loud crash.

I assume you mean before this patch right? If so, then yes and no. It's possible the constant location is in a readonly page, but I have a lot of downstream constant locations which are intermixed with writeable data. I'm sure other frontends might have the same. Your reasoning may hold specifically for global constants though.

Mon, Apr 15, 8:15 AM · Restricted Project

Sat, Apr 13

reames updated the diff for D60659: [InstCombine] Eliminate stores to constant memory.

Rebase on top of test changes

Sat, Apr 13, 3:13 PM · Restricted Project
reames committed rG0eeb2cd491bd: [Tests] Add tests for D60659, and make adjustments to others to make diff clear (authored by reames).
[Tests] Add tests for D60659, and make adjustments to others to make diff clear
Sat, Apr 13, 3:12 PM
reames committed rL358344: [Tests] Add tests for D60659, and make adjustments to others to make diff clear.
[Tests] Add tests for D60659, and make adjustments to others to make diff clear
Sat, Apr 13, 3:11 PM
reames created D60659: [InstCombine] Eliminate stores to constant memory.
Sat, Apr 13, 2:54 PM · Restricted Project

Fri, Apr 12

reames added a comment to D59020: [StackMaps] Update llvm-readobj to parse V3 Stackmaps.

I tried applying the patch again, and discovered the problem. Phabricator was stripping out the binary portions, so I wasn't getting the test changes. (I'd also seen a build failure last time, but didn't see that again.)

Fri, Apr 12, 8:56 PM · Restricted Project
reames committed rGe03301a3b32d: [StackMaps] Update llvm-readobj to parse V3 Stackmaps (authored by reames).
[StackMaps] Update llvm-readobj to parse V3 Stackmaps
Fri, Apr 12, 8:55 PM
reames committed rL358325: [StackMaps] Update llvm-readobj to parse V3 Stackmaps.
[StackMaps] Update llvm-readobj to parse V3 Stackmaps
Fri, Apr 12, 8:54 PM
reames closed D59020: [StackMaps] Update llvm-readobj to parse V3 Stackmaps.
Fri, Apr 12, 8:54 PM · Restricted Project
reames committed rGeea989a909a3: [StackMaps] Add location size to llvm-readobj -stackmap output (authored by reames).
[StackMaps] Add location size to llvm-readobj -stackmap output
Fri, Apr 12, 8:07 PM
reames committed rL358324: [StackMaps] Add location size to llvm-readobj -stackmap output.
[StackMaps] Add location size to llvm-readobj -stackmap output
Fri, Apr 12, 8:07 PM
reames closed D59169: [StackMaps] Add location size to llvm-readobj -stackmap output.
Fri, Apr 12, 8:07 PM · Restricted Project
reames committed rGf7acef9c88f7: [llvm-readobj] Minor style tweak for consistency sake [NFC] (authored by reames).
[llvm-readobj] Minor style tweak for consistency sake [NFC]
Fri, Apr 12, 7:25 PM
reames committed rL358323: [llvm-readobj] Minor style tweak for consistency sake [NFC].
[llvm-readobj] Minor style tweak for consistency sake [NFC]
Fri, Apr 12, 7:21 PM
reames committed rG377f507a9ffd: [StackMaps] Remove format version from the class name [NFC] (authored by reames).
[StackMaps] Remove format version from the class name [NFC]
Fri, Apr 12, 7:03 PM
reames committed rL358322: [StackMaps] Remove format version from the class name [NFC].
[StackMaps] Remove format version from the class name [NFC]
Fri, Apr 12, 7:01 PM
reames committed rGcebf0b3ab544: [StackMaps] Add explicit location size accessor to the stackmap parser (authored by reames).
[StackMaps] Add explicit location size accessor to the stackmap parser
Fri, Apr 12, 6:50 PM
reames committed rL358319: [StackMaps] Add explicit location size accessor to the stackmap parser.
[StackMaps] Add explicit location size accessor to the stackmap parser
Fri, Apr 12, 6:50 PM
reames closed D59167: [StackMaps] Add explicit location size accessor to the stackmap parser.
Fri, Apr 12, 6:49 PM · Restricted Project
reames added a comment to D60606: [SimpleLoopUnswitch] Implement handling of prof branch_weights metadata for SwitchInst.

Two additional comments:

  1. We appear to have the same lost profile problem for partial unswitching of branches.
  2. When I wrote my early comment, I didn't realize this was sitting on top of proposed API changes. I'd suggest focusing on the earlier patches, then wan return to this one.
Fri, Apr 12, 11:57 AM · Restricted Project
reames requested changes to D60554: Fix number of args of prof branch_weights MD for SwitchInst.
Fri, Apr 12, 11:56 AM · Restricted Project
reames requested changes to D60604: [IR] Improve SwitchInst API to support prof branch_weights.

I'm not entirely sure about this interface. I see there's precedent in code for working with the branch weights when removing cases, so I see where you're coming from, but the rest of our interface (e.g. branches) don't have an analogous API.

Fri, Apr 12, 11:49 AM · Restricted Project
reames added a comment to D60606: [SimpleLoopUnswitch] Implement handling of prof branch_weights metadata for SwitchInst.

The code change looks reasonable to me, but I'd like to let Chandler confirm since he's more familiar with the code.

Fri, Apr 12, 11:41 AM · Restricted Project
reames added a comment to rL356510: Demanded elements support for masked.load and masked.gather.

Be aware that this patch turned out to contain a nasty miscompile. It's now been fixed in https://reviews.llvm.org/rL358299. I recommend anyone who has branched with this change to consider cherry-picking the fix.

Fri, Apr 12, 11:32 AM
reames added a comment to D57372: Demanded elements support for masked.load and masked.gather.

Be aware that this patch turned out to contain a nasty miscompile. It's now been fixed in https://reviews.llvm.org/rL358299. I recommend anyone who has branched with this change to consider cherry-picking the fix.

Fri, Apr 12, 11:32 AM · Restricted Project
reames committed rGb091cc081df4: [InstCombine] Fix a nasty miscompile introduced w/masked.gather demanded elts (authored by reames).
[InstCombine] Fix a nasty miscompile introduced w/masked.gather demanded elts
Fri, Apr 12, 11:27 AM
reames committed rL358299: [InstCombine] Fix a nasty miscompile introduced w/masked.gather demanded elts.
[InstCombine] Fix a nasty miscompile introduced w/masked.gather demanded elts
Fri, Apr 12, 11:27 AM
reames committed rG7a60cd38af55: [Tests] Checkin a test demonstrating a miscompile so that patch which fixes it… (authored by reames).
[Tests] Checkin a test demonstrating a miscompile so that patch which fixes it…
Fri, Apr 12, 11:11 AM
reames committed rL358296: [Tests] Checkin a test demonstrating a miscompile so that patch which fixes it….
[Tests] Checkin a test demonstrating a miscompile so that patch which fixes it…
Fri, Apr 12, 11:10 AM

Thu, Apr 11

reames added a comment to D60056: Hoist/sink malloc/free's in LICM..

minor comments on interface, LICM part, and tests. I got interrupted and need to come back and finish reviewing the analysis implementation.

Thu, Apr 11, 9:38 AM · Restricted Project

Mon, Apr 8

reames abandoned D60098: [POC] Iterative hoisting during trivial unswitch.

Abandoning this approach. As Chandler pointed out in offline discussion, the iteration of the loop pass manager itself - remember, this is in the *new* pass manager, not the old - should handle this case. I'm going to try to figure out why it didn't first, and then only return to this approach if I'm either a) stuck on that for some reason or b) trying to improve efficiency (i.e. reduce number of iterations required.)

Mon, Apr 8, 10:27 AM