Page MenuHomePhabricator

gilr (Gil Rapaport)
User

Projects

User does not belong to any projects.

User Details

User Since
Nov 12 2014, 11:00 PM (344 w, 4 d)

Recent Activity

Thu, Jun 17

gilr added a comment to D103700: [LV] Fix bug when unrolling (only) a loop with non-latch exit.

@Ayal If I'm understanding you even partially correctly, it sounds like you're raising a code quality issue. That is, we may generate a dead epilogue loop (e.g. with a condition statically known to result in the epilogue being untaken, but emitted as a condition) when we didn't need to. Is that correct?

Thu, Jun 17, 10:10 AM · Restricted Project

May 18 2021

gilr accepted D100257: [VPlan] Add VPUserID to distinguish between recipes and others..

@gilr I think your comments should be addressed and I am planning land this in the next few days, unless there are any additional comments :)

May 18 2021, 12:31 AM · Restricted Project

May 10 2021

gilr added inline comments to D100257: [VPlan] Add VPUserID to distinguish between recipes and others..
May 10 2021, 6:34 AM · Restricted Project
gilr added a comment to D100257: [VPlan] Add VPUserID to distinguish between recipes and others..

Thanks, I updated the code to turn VPUser in a virtual base class and added s subclass for the uses in VPBlockBase. WDYT?

May 10 2021, 4:03 AM · Restricted Project

May 3 2021

gilr added a comment to D100258: [VPlan] Add first VPlan version of sinkScalarOperands..

Very nice step forward!!

May 3 2021, 8:22 AM · Restricted Project
gilr added a comment to D100257: [VPlan] Add VPUserID to distinguish between recipes and others..

Non-recipe "other" VPUsers are quite exceptional ... would it suffice to have (native) VPlan hold all 'dangling' non-recipe VPUsers of its VPBasicBlocks, as an alternative, until these are cleaned up?

The blocks need to use the VPUsers to access the current value they hold I think, so I am not sure how the VPlan holding the VPUsers would look like unfortunately. It would be great if you could elaborate in a bit more detail, in case I am missing something.

The blocks will still have their VPUsers (in native; until this gets cleaned up). The thought is that whoever needs to distinguish between a block'd VPUser and a recipe'd one, would do so by consulting VPlan, rather than isa-ing the VPUser.

Oh I see what you mean now, thanks! That sounds like a viable alternative.

IIUC we will have non-recipe users in the future (e.g. live-outs) so I think we need to be able to make this distinction in the future as well, once the current VPUsers in the native path get cleaned up. If that's true, I think having to consult a VPlan to check what type of user we are dealing with seems a bit clunky in the long term (and breaks with the common pattern used for other VP* classes and other places in LLVM). Unless we anticipate a more fine-grained distinction between VPUsers in the future, we could also just use a boolean flag instead of the enum, to make it less appealing to add new VPUser types.

If we don't anticipate having to distinguish between recipe and non-recipe VPUsers in the future, then I think using a map in the plan directly makes a lot of sense (and in that case we might want to get rid of the separate VPUser class completely?)

May 3 2021, 2:48 AM · Restricted Project

Apr 20 2021

gilr added inline comments to D100258: [VPlan] Add first VPlan version of sinkScalarOperands..
Apr 20 2021, 2:36 AM · Restricted Project

Feb 19 2021

gilr accepted D95383: [LV] Remove VPCallback..

LGTM, thanks!

Feb 19 2021, 3:40 AM · Restricted Project

Feb 18 2021

gilr added a comment to D95383: [LV] Remove VPCallback..

ping :)

Feb 18 2021, 11:46 AM · Restricted Project

Feb 15 2021

gilr accepted D92285: [VPlan] Manage scalarized values using VPValues..
Feb 15 2021, 2:43 PM · Restricted Project

Feb 9 2021

gilr added inline comments to D95383: [LV] Remove VPCallback..
Feb 9 2021, 9:33 AM · Restricted Project
gilr added inline comments to D92285: [VPlan] Manage scalarized values using VPValues..
Feb 9 2021, 3:55 AM · Restricted Project

Feb 8 2021

gilr accepted D95382: [VPlan] Use VPUser to manage CondBit.
Feb 8 2021, 1:25 PM · Restricted Project

Feb 7 2021

gilr added inline comments to D95382: [VPlan] Use VPUser to manage CondBit.
Feb 7 2021, 1:44 PM · Restricted Project
gilr accepted D95757: [LV] Replace some uses of VectorLoopValueMap with VPTransformState (NFC).

LGTM, thanks!

Feb 7 2021, 9:21 AM · Restricted Project

Feb 3 2021

gilr added inline comments to D92285: [VPlan] Manage scalarized values using VPValues..
Feb 3 2021, 1:18 PM · Restricted Project
gilr added inline comments to D95757: [LV] Replace some uses of VectorLoopValueMap with VPTransformState (NFC).
Feb 3 2021, 10:46 AM · Restricted Project
gilr accepted D92284: [VPlan] Manage induction value creation using VPValues..

LGTM with a nit. Thanks!

Feb 3 2021, 8:41 AM · Restricted Project

Feb 1 2021

gilr committed rGd475030dc28a: [SCEV] Apply loop guards to divisibility tests (authored by gilr).
[SCEV] Apply loop guards to divisibility tests
Feb 1 2021, 10:11 PM
gilr closed D95521: [SCEV] Apply loop guards to divisibility tests.
Feb 1 2021, 10:10 PM · Restricted Project
gilr retitled D95521: [SCEV] Apply loop guards to divisibility tests from [SCEV] Apply loop guards to zero modulo conditions to [SCEV] Apply loop guards to divisibility tests.
Feb 1 2021, 9:29 PM · Restricted Project
gilr updated the diff for D95521: [SCEV] Apply loop guards to divisibility tests.

Addressed comments.

Feb 1 2021, 11:14 AM · Restricted Project
gilr added a comment to D95521: [SCEV] Apply loop guards to divisibility tests.

I did some testing and it appears this exposes a crash in matchURem. I'm taking a look at that now, I think it would be good to wait with landing the change until this is resolved.

Should be fixed by f1e8136115ac

Feb 1 2021, 8:45 AM · Restricted Project

Jan 31 2021

gilr updated the diff for D95521: [SCEV] Apply loop guards to divisibility tests.

Addressed comments.

Jan 31 2021, 11:22 AM · Restricted Project
gilr added inline comments to D95521: [SCEV] Apply loop guards to divisibility tests.
Jan 31 2021, 10:11 AM · Restricted Project
gilr updated the diff for D95521: [SCEV] Apply loop guards to divisibility tests.

Rebased

Jan 31 2021, 1:21 AM · Restricted Project

Jan 27 2021

gilr added inline comments to D94088: [SCEV] Assumption context for GetMinTrailingZeros.
Jan 27 2021, 8:08 AM · Restricted Project
gilr added a comment to D94088: [SCEV] Assumption context for GetMinTrailingZeros.

Trying to apply Florian's suggestion in D95521 (only for powers of 2 as a first step). This handles the power-of-two case added by PR47904.

Jan 27 2021, 6:05 AM · Restricted Project
gilr requested review of D95521: [SCEV] Apply loop guards to divisibility tests.
Jan 27 2021, 6:04 AM · Restricted Project

Jan 25 2021

gilr added a comment to D94088: [SCEV] Assumption context for GetMinTrailingZeros.

I would expect that the SCEV change could be testable with just the scev itselfs (-analyze -scalar-evolution), is it not?

Jan 25 2021, 4:27 PM · Restricted Project
gilr added a comment to D94088: [SCEV] Assumption context for GetMinTrailingZeros.

ping

Jan 25 2021, 3:20 AM · Restricted Project

Jan 24 2021

gilr accepted D92282: [VPlan] Handle scalarized values in VPTransformState..
Jan 24 2021, 11:48 PM · Restricted Project
gilr requested changes to D92282: [VPlan] Handle scalarized values in VPTransformState..

LGTM, with a nit. Thanks!

Jan 24 2021, 11:47 PM · Restricted Project

Jan 23 2021

gilr added inline comments to D92282: [VPlan] Handle scalarized values in VPTransformState..
Jan 23 2021, 11:59 PM · Restricted Project

Jan 19 2021

gilr added a comment to D92284: [VPlan] Manage induction value creation using VPValues..

Should the (final) goal be to move the whole phi-trunc-cast logic from code-gen to vplanning phase?

Jan 19 2021, 2:39 PM · Restricted Project
gilr added inline comments to D92282: [VPlan] Handle scalarized values in VPTransformState..
Jan 19 2021, 7:37 AM · Restricted Project

Jan 13 2021

gilr added a comment to D93974: [ValueTracking] Safe assumption context for args.

Oh.. this is bad. That happens if you mix two logically differnet things into a single instruction pointer.
I put it on the list of things that need to fixed wrt. assumes ... :(

Jan 13 2021, 2:30 PM · Restricted Project

Jan 12 2021

gilr added a comment to D93974: [ValueTracking] Safe assumption context for args.

Yes, if isaAssumeIntrinsic stands for "is an assume or an ephemeral of an assume".

Why is anything but an llvm.assume a problem? (Sorry I have so many questions)

Jan 12 2021, 1:41 PM · Restricted Project
gilr added a comment to D93974: [ValueTracking] Safe assumption context for args.

The idea is that you can always do what you do here in isValidAssumeForContext (and friends). I just checked and we seem to do so already. Could you explain to me why we need to go for the Last instruction in this patch at all? What would happen if you simply pick the first in the entry block, which is trivially correct. (Note: You can skip llvm.assume to make some weird problems go away).

The patch indeed defaults to the first instruction in the entry. Scanning to the end at safeCxtI() was an optimization for cases where the first instruction is an ephemeral value of an assume (which seems quite likely for assumes about arguments) that would get the assume discarded by the isEphemeralValueOf(Inv, CxtI) check. At isValidAssumeForContext() we can't distinguish between a context given only as a control-flow marker and a context that also guards against simplifying an ephemeral so we can't try to improve it there, but since any context safeCxtI() provides for a null CxtI is just a control-flow marker anyway we might as well choose one that's not an ephemeral of any assume in the entry block. Does that make sense?

Yes. But your code does "more" than that. All you want is this pseudo-code, right?

auto &It = EntryBlock.begin();
while (isaAssumeIntrinsic(*It)) ++It;
return *It;
Jan 12 2021, 10:24 AM · Restricted Project
gilr added a comment to D93974: [ValueTracking] Safe assumption context for args.
Jan 12 2021, 9:33 AM · Restricted Project
gilr added a comment to D94088: [SCEV] Assumption context for GetMinTrailingZeros.

Ping :)

Jan 12 2021, 4:46 AM · Restricted Project
gilr added a comment to D93974: [ValueTracking] Safe assumption context for args.

FWIW, I agree with @nikic, we should not put this logic here. There are two problems:

  1. We compute something we might not need.
  2. We do it only in value tracking.
Jan 12 2021, 3:57 AM · Restricted Project

Jan 8 2021

gilr accepted D93975: [VPlan] Keep start value of VPWidenPHIRecipe as VPValue..

LGTM!

Jan 8 2021, 11:24 PM · Restricted Project
gilr accepted D94175: [VPlan] Move reduction start value creation to widenPHIRecipe..

LGTM, with a nit.
(Seems reduction might be worth splitting widenPhiInstruction() and its own Recipe, but that's beyond the scope of this patch)
Thanks!

Jan 8 2021, 8:40 AM · Restricted Project

Jan 7 2021

gilr updated the diff for D94088: [SCEV] Assumption context for GetMinTrailingZeros.

Rebased on top of 7ddbe0cb905ec62d37b284a2e8daf54887a35f94 (merge ..-assume-divisible-TC.ll into ..-divisible-TC.ll)

Jan 7 2021, 12:36 AM · Restricted Project

Jan 6 2021

gilr committed rG7ddbe0cb905e: [LV] Merge tests into a single file (NFC) (authored by gilr).
[LV] Merge tests into a single file (NFC)
Jan 6 2021, 11:12 PM
gilr updated the diff for D94088: [SCEV] Assumption context for GetMinTrailingZeros.

Addressed comments.

Jan 6 2021, 1:01 AM · Restricted Project
gilr added inline comments to D94088: [SCEV] Assumption context for GetMinTrailingZeros.
Jan 6 2021, 12:56 AM · Restricted Project

Jan 5 2021

gilr added a comment to D93974: [ValueTracking] Safe assumption context for args.

Sorry for the delay @nikic.

Am I understanding correctly that this tries to use the last instruction in the entry block rather than the first one to avoid triggering the ephemeral value check, in case the first instruction is part of an assumption?

Yes, the first instruction is fine except for an assume which appears (along with its ephemeral values) as the first thing in the function, which in case of an argument seems quite likely.

In any case, I don't think it's appropriate to perform a full block scan to determine the context instruction. safeCxtI() should be cheap (as in O(1)).

An unbounded scan of the entry block might indeed be too much even if applied only to arguments. The scan can perhaps be limited to some small number of instructions (5 seems like the minimum to cover most patterns in computeKnownBits()) which gets reset if an assume is encountered.

Another possibility would be to leave the context instruction at nullptr, and instead adjust isValidAssumeForContext to accept a nullptr CxtI in which case only instructions that are must-exec from the function entry are considered. Advantage is that it only incurs a cost if there is a potentially relevant assume.

Jan 5 2021, 1:43 PM · Restricted Project
gilr added a comment to D93975: [VPlan] Keep start value of VPWidenPHIRecipe as VPValue..

I wonder if the code from fixReductions() could be moved (almost as-is) to widenPHIInstruction() as a first step?

Jan 5 2021, 11:28 AM · Restricted Project
gilr updated subscribers of D94088: [SCEV] Assumption context for GetMinTrailingZeros.
Jan 5 2021, 8:20 AM · Restricted Project
gilr requested review of D94088: [SCEV] Assumption context for GetMinTrailingZeros.
Jan 5 2021, 7:34 AM · Restricted Project
gilr added a comment to D93974: [ValueTracking] Safe assumption context for args.

Sorry for the delay @nikic.

Jan 5 2021, 7:01 AM · Restricted Project

Jan 3 2021

gilr committed rGd9c0b128e354: [SCEV] Simplify trunc to zero based on known bits (authored by gilr).
[SCEV] Simplify trunc to zero based on known bits
Jan 3 2021, 4:08 AM
gilr closed D93973: [SCEV] Simplify trunc to zero based on known bits.
Jan 3 2021, 4:08 AM · Restricted Project
gilr added inline comments to D93973: [SCEV] Simplify trunc to zero based on known bits.
Jan 3 2021, 3:54 AM · Restricted Project

Jan 2 2021

gilr updated the diff for D93973: [SCEV] Simplify trunc to zero based on known bits.

Limit trailing-zeros check to SCEVUnknowns and only while depth limit is not reached.

Jan 2 2021, 11:31 PM · Restricted Project
gilr added inline comments to D93973: [SCEV] Simplify trunc to zero based on known bits.
Jan 2 2021, 11:27 PM · Restricted Project
gilr updated the diff for D93973: [SCEV] Simplify trunc to zero based on known bits.

Addressed comments.

Jan 2 2021, 7:45 AM · Restricted Project
gilr committed rGd8af31006351: [LV] Add missed optimization fold-tail test (authored by gilr).
[LV] Add missed optimization fold-tail test
Jan 2 2021, 4:02 AM
gilr requested review of D93974: [ValueTracking] Safe assumption context for args.
Jan 2 2021, 12:05 AM · Restricted Project

Jan 1 2021

gilr requested review of D93973: [SCEV] Simplify trunc to zero based on known bits.
Jan 1 2021, 11:59 PM · Restricted Project

Dec 22 2020

gilr accepted D93677: [LV] Use ScalarEvolution::getURemExpr to reduce duplication..

LGTM, thanks!

Dec 22 2020, 5:49 AM · Restricted Project
gilr committed rGa56280094e08: [LV] Avoid needless fold tail (authored by gilr).
[LV] Avoid needless fold tail
Dec 22 2020, 12:26 AM
gilr closed D93615: [LV] Avoid needless fold tail.
Dec 22 2020, 12:26 AM · Restricted Project
gilr added inline comments to D93615: [LV] Avoid needless fold tail.
Dec 22 2020, 12:02 AM · Restricted Project

Dec 21 2020

gilr updated the diff for D93615: [LV] Avoid needless fold tail.

Add a test for a constant TC with IC=3.

Dec 21 2020, 5:42 AM · Restricted Project
gilr added inline comments to D93615: [LV] Avoid needless fold tail.
Dec 21 2020, 5:33 AM · Restricted Project
gilr added a reviewer for D93615: [LV] Avoid needless fold tail: SjoerdMeijer.
Dec 21 2020, 12:18 AM · Restricted Project

Dec 20 2020

gilr requested review of D93615: [LV] Avoid needless fold tail.
Dec 20 2020, 11:29 PM · Restricted Project
gilr accepted D90562: [VPlan] Use VPDef for VPInterleaveRecipe..

LGTM!

Dec 20 2020, 10:38 PM · Restricted Project
gilr added inline comments to D90562: [VPlan] Use VPDef for VPInterleaveRecipe..
Dec 20 2020, 1:57 AM · Restricted Project

Dec 16 2020

gilr added inline comments to D90562: [VPlan] Use VPDef for VPInterleaveRecipe..
Dec 16 2020, 5:23 AM · Restricted Project
gilr added inline comments to D90562: [VPlan] Use VPDef for VPInterleaveRecipe..
Dec 16 2020, 12:49 AM · Restricted Project

Dec 15 2020

gilr accepted D90560: [VPlan] Use VPDef for VPWidenSelectRecipe..

LGTM, tx!

Dec 15 2020, 5:42 AM · Restricted Project

Dec 14 2020

gilr added a comment to D90560: [VPlan] Use VPDef for VPWidenSelectRecipe..

How is the new test related to this change?

Dec 14 2020, 11:16 PM · Restricted Project
gilr accepted D90559: [VPlan] Use VPdef for VPWidenCall..
Dec 14 2020, 11:12 PM · Restricted Project
gilr accepted D90561: [VPlan] Use VPDef for VPWidenGEPRecipe..
Dec 14 2020, 11:12 PM · Restricted Project
gilr accepted D90563: [VPlan] Make VPWidenMemoryInstructionRecipe a VPDef..

LGTM, tx!

Dec 14 2020, 5:25 AM · Restricted Project
gilr added inline comments to D90563: [VPlan] Make VPWidenMemoryInstructionRecipe a VPDef..
Dec 14 2020, 2:58 AM · Restricted Project

Nov 28 2020

gilr accepted D90565: [VPlan] Make VPInstruction a VPDef.

LGTM, tx!

Nov 28 2020, 9:47 AM · Restricted Project

Nov 24 2020

gilr accepted D90564: [VPlan] Make VPRecipeBase inherit from VPDef..

LGTM, tx!

Nov 24 2020, 9:08 AM · Restricted Project
gilr requested changes to D90564: [VPlan] Make VPRecipeBase inherit from VPDef..
Nov 24 2020, 6:09 AM · Restricted Project

Nov 22 2020

gilr added inline comments to D90564: [VPlan] Make VPRecipeBase inherit from VPDef..
Nov 22 2020, 5:33 AM · Restricted Project

Nov 15 2020

gilr added inline comments to D90565: [VPlan] Make VPInstruction a VPDef.
Nov 15 2020, 3:05 AM · Restricted Project

Jul 8 2020

gilr added inline comments to D75069: [LoopVectorizer] Inloop vector reductions.
Jul 8 2020, 6:42 AM · Restricted Project

Jun 16 2020

gilr accepted D80220: [VPlan] Add & use VPValue for VPWidenGEPRecipe operands (NFC)..

We still use GEP->clone() for the case where the GEP is loop invariant. It would probably be best to do that in a follow-on patch. Same for perhaps storing InBounds directly in the recipe as well.

Jun 16 2020, 2:43 AM · Restricted Project

Jun 6 2020

gilr added a comment to D80220: [VPlan] Add & use VPValue for VPWidenGEPRecipe operands (NFC)..

The patch LGTM, any reason not to include the base pointer?

Jun 6 2020, 11:12 AM · Restricted Project

May 24 2020

gilr accepted D80219: [VPlan] Use VPUser for VPWidenSelectRecipe operands (NFC)..
May 24 2020, 1:02 AM · Restricted Project

May 18 2020

gilr accepted D80114: [VPlan] Add & use VPValue operands for VPReplicateRecipe (NFC)..

LGTM with a nit. Thanks!

May 18 2020, 8:01 AM · Restricted Project

May 10 2020

gilr accepted D78883: [VPlan] Move emission of \\l\"+\n to dumpBasicBlock (NFC)..

Thanks for looking into this, Florian (and sorry for taking so long).
LGTM, except VPInterleaveRecipe::print() is missing here.
The patch stands by itself as a standardization of printing, similar to how the multi-line Function and Module print. If meant as a first step, would be good to elaborate on the full picture.

May 10 2020, 12:29 AM · Restricted Project

Apr 22 2020

gilr accepted D76992: [VPlan] Add & use VPValue operands for VPWidenRecipe (NFC)..

LGTM (with that missing comment nit), thanks!

Apr 22 2020, 8:06 AM · Restricted Project
gilr added inline comments to D76992: [VPlan] Add & use VPValue operands for VPWidenRecipe (NFC)..
Apr 22 2020, 4:49 AM · Restricted Project
gilr added inline comments to D76992: [VPlan] Add & use VPValue operands for VPWidenRecipe (NFC)..
Apr 22 2020, 4:17 AM · Restricted Project
gilr added inline comments to D76992: [VPlan] Add & use VPValue operands for VPWidenRecipe (NFC)..
Apr 22 2020, 3:44 AM · Restricted Project

Apr 19 2020

gilr accepted D78288: [VPlan] Make various tryTo* helpers private and mark as const (NFC)..

LGTM, thanks.

Apr 19 2020, 3:11 AM · Restricted Project

Apr 15 2020

gilr committed rGb747d72c1971: [LV] Fix PR45525: Incorrect assert in blend recipe (authored by gilr).
[LV] Fix PR45525: Incorrect assert in blend recipe
Apr 15 2020, 1:03 AM
gilr closed D78115: [LV] Fix PR45525: Incorrect assert in blend recipe.
Apr 15 2020, 1:03 AM · Restricted Project
gilr updated the diff for D78115: [LV] Fix PR45525: Incorrect assert in blend recipe.

Addressed comments and narrowed LIT checks to the relevant vectorized instructions.
Thanks @fhahn !

Apr 15 2020, 12:30 AM · Restricted Project

Apr 14 2020

gilr created D78115: [LV] Fix PR45525: Incorrect assert in blend recipe.
Apr 14 2020, 8:33 AM · Restricted Project