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

In D93974#2494331, @jdoerfert wrote:

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 ... :(

In D93974#2493723, @jdoerfert wrote:

In D93974#2493646, @gilr wrote:

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)

In D93974#2493403, @jdoerfert wrote:

In D93974#2493287, @gilr wrote:

In D93974#2492979, @jdoerfert wrote:

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;

In D93974#2492979, @jdoerfert wrote:

Ping :)

In D93974#2489195, @jdoerfert wrote:

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.

LGTM!

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!

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

Addressed comments.

In D93974#2480270, @nikic wrote:

In D93974#2479403, @gilr wrote:

Sorry for the delay @nikic.

In D93974#2476165, @nikic wrote:

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.

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

Sorry for the delay @nikic.

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

Addressed comments.

LGTM, thanks!

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

LGTM!

LGTM, tx!

How is the new test related to this change?

LGTM, tx!

LGTM, tx!

LGTM, tx!

In D80220#2078460, @fhahn wrote:

In D80220#2078222, @gilr wrote:

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.

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

LGTM with a nit. Thanks!

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.

LGTM (with that missing comment nit), thanks!

LGTM, thanks.

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

LGTM, thanks!

Addressed comments.

Address comments

LGTM, thanks!

LGTM, with a nit.

Addressed comments.

In D76373#1939483, @fhahn wrote:

LGTM, thanks! Please wait a day or so with committing, in case there are other comments.

Addressed comments.

Ping :)

Clang format.

In D74695#1923526, @fhahn wrote:

I think doing so for VPWidenRecipe might be more tricky than other recipes, as VPWidenRecipe currently contains a list of instructions.
[snip]
I thought a little bit about how to migrate the def-use dependencies for VPWidenRecipe without breaking it up, but couldn't come up with a more straight-forward approach.

LGTM!

In D73078#1903249, @fhahn wrote:

In D73078#1903177, @gilr wrote:

In D73078#1902767, @fhahn wrote:

In D73078#1902664, @gilr wrote:

[snip]
I think we also introduce some VPInstructions not connected to any underlying IR during predication and will add more in the future. I think we should do (and need) both, using the IR name when available and the slot tracker otherwise. I think it would be good to have using the underlying IR names as a follow up to this patch. What do you think?

After D74445 all VPInstructions can indeed print themselves by either using their IR name or defaulting to SlotTracker/badref. So this patch can be simplified by not propagating the SlotTracker through the print() methods, since non-VPInstruction VPValues can rely on their underlying IR values for printing. Note that for this to work addVPValue must plant the underlying IR value into the generated VPValue as you do in D74695.
BackedgeTakenCount would then remain the only user of the current pointer-based naming and consequently of the "where:" clause.

Ah I think I got what you are proposing now: just remove the ::print(raw_ostream &O, const Twine &Indent, VPSlotTracker &SlotTracker) variants taking a SlotTracker, because we can just instantiate it on demand, by just getting the parent plan for the containing VPBB. This means we might have to re-number the plan multiple times when printing the whole plan through VPlanPrinter, but as this is just for debugging (and the scope of a plan should be relatively small) we can adjust that as follow-up, if required.

So the recursive print of any VP entity should use a single, up-to-date SlotTracker. This SlotTracker may be generated/assigned when entering the recursion (e.g. on operator<< or print()) and destroyed/invalidated when leaving it. One could optimize later to invalidate only when needed, i.e. on VPlan changes.

I think I am missing something.

I think this is exactly what the patch does: when calling print without SlotTracker, it will be created on demand and VPlanPrinter re-uses the same slot tracker for printing all VP entities by passing it to print(). I'm not sure what I should change, besides a follow-up to use the underlying values, if available/

In D73078#1902767, @fhahn wrote:

In D73078#1902664, @gilr wrote:

[snip]
I think we also introduce some VPInstructions not connected to any underlying IR during predication and will add more in the future. I think we should do (and need) both, using the IR name when available and the slot tracker otherwise. I think it would be good to have using the underlying IR names as a follow up to this patch. What do you think?

After D74445 all VPInstructions can indeed print themselves by either using their IR name or defaulting to SlotTracker/badref. So this patch can be simplified by not propagating the SlotTracker through the print() methods, since non-VPInstruction VPValues can rely on their underlying IR values for printing. Note that for this to work addVPValue must plant the underlying IR value into the generated VPValue as you do in D74695.
BackedgeTakenCount would then remain the only user of the current pointer-based naming and consequently of the "where:" clause.

Ah I think I got what you are proposing now: just remove the ::print(raw_ostream &O, const Twine &Indent, VPSlotTracker &SlotTracker) variants taking a SlotTracker, because we can just instantiate it on demand, by just getting the parent plan for the containing VPBB. This means we might have to re-number the plan multiple times when printing the whole plan through VPlanPrinter, but as this is just for debugging (and the scope of a plan should be relatively small) we can adjust that as follow-up, if required.

In D73078#1900122, @fhahn wrote:

Updated patch to use getPlan(), add unit tests.

In D73078#1899992, @gilr wrote:

In D73078#1870844, @fhahn wrote:

[snip]
I've put up a patch to add a pointer to VPlan: D74445. I;ve also added implementations of print() with and without SlotTracker (so we can re-use the existing tracker, e.g. from VPlanPrinter. I still need to update the slot tracker to actually use it. VPInstructions without a block (or a VPBasicBlock without parentPlan) should probably be handled similar to IR instructions without BB, by using something like 'badref'.

What do you think?

This added pointer to VPlan can indeed take care of enumerating VPInstructions, defaulting to 'badref'.
Essentially all other VPValues except BackedgeTakenCount are wrappers of underlying IR values (right?), so they can print that value e.g. with the proposed %ir. w/o needing SlotTracker's services, thereby simplifying the patch. This would leave the somewhat special BackedgeTakenCount as the only user of the "where:" clause and current pointer-based naming.

I think we also introduce some VPInstructions not connected to any underlying IR during predication and will add more in the future. I think we should do (and need) both, using the IR name when available and the slot tracker otherwise. I think it would be good to have using the underlying IR names as a follow up to this patch. What do you think?