Page MenuHomePhabricator

[VPlan] Add & use VPValue for VPWidenGEPRecipe operands (NFC).
ClosedPublic

Authored by fhahn on May 19 2020, 9:17 AM.

Details

Summary

This patch adds VPValue version of the GEP's operands to
VPWidenGEPRecipe and uses them during code-generation.

Diff Detail

Event Timeline

fhahn created this revision.May 19 2020, 9:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2020, 9:17 AM
gilr added a comment.Sat, Jun 6, 10:46 AM

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

fhahn updated this revision to Diff 269056.Sun, Jun 7, 7:46 AM

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

I initially thought it might make things easier to do base ptr/indices separately, but it turned out handling the base ptr in the patch as well was not much extra work. Updated!

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.

fhahn retitled this revision from [VPlan] Add & use VPValue indices for VPWidenGEPRecipe (NFC). to [VPlan] Add & use VPValue for VPWidenGEPRecipe operands (NFC)..Sun, Jun 7, 9:06 AM
fhahn edited the summary of this revision. (Show Details)
gilr accepted this revision.Tue, Jun 16, 2:31 AM

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.

Right, clone() should be replaced by generating the GEP from scratch as done in the non-invariant case. Also, now that we have VPOperands it would be better to expose an VPValue::isInvariant() / VPlan::isInvariant(VPValue &) API instead of recording that datum in WidenGEP & WidenSelect recipes, facilitating RAUW. Such API could initially query the underlying IR.
Other properties already captured by an underlying value such as inBounds and nsw are probably worth a separate discussion.
All these can indeed be follow-ups.

This revision is now accepted and ready to land.Tue, Jun 16, 2:31 AM
bmahjour removed a subscriber: bmahjour.Tue, Jun 16, 6:33 AM
fhahn added a comment.Fri, Jun 26, 1:00 PM

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.

Right, clone() should be replaced by generating the GEP from scratch as done in the non-invariant case. Also, now that we have VPOperands it would be better to expose an VPValue::isInvariant() / VPlan::isInvariant(VPValue &) API instead of recording that datum in WidenGEP & WidenSelect recipes, facilitating RAUW. Such API could initially query the underlying IR.
Other properties already captured by an underlying value such as inBounds and nsw are probably worth a separate discussion.
All these can indeed be follow-ups.

Thanks Gil! I'll prepare follow-up patches in a bit.

This revision was automatically updated to reflect the committed changes.