Page MenuHomePhabricator

[SCEV] SCEVExpander::isHighCostExpansion(): assert if TTI is not provided
ClosedPublic

Authored by lebedev.ri on Jan 30 2020, 7:44 AM.

Details

Summary

Currently, as per check-llvm, we never call SCEVExpander::isHighCostExpansion() with null TTI,
so this appears to be a safe restriction.

Diff Detail

Event Timeline

lebedev.ri created this revision.Jan 30 2020, 7:44 AM

Rebased, NFC.

I'm not sure about how we should interpret lack of TTI. Maybe we should interpret it like "if there is no model, we don't care about the high costs, and everything is low cost enough", or opposite (like in your patch). Neither of this looks NFC to me (given that we can pass null TTI). Maybe we should merge it *after* we make use of TTI inside the methods?

I'm not sure about how we should interpret lack of TTI.
Maybe we should interpret it like "if there is no model, we don't care about the high costs, and everything is low cost enough", or opposite (like in your patch).

Ack, this is non-obvious.

Neither of this looks NFC to me (given that we can pass null TTI).

Alternatively, we can simply require TTI - i believe at least in all the current callsites, it *will* be provided.

Maybe we should merge it *after* we make use of TTI inside the methods?

We have to account for the null possibility somehow, if we don't we would just be adding potential null pointer dereference.

lebedev.ri updated this revision to Diff 242586.Feb 5 2020, 6:22 AM
lebedev.ri retitled this revision from [SCEV] SCEVExpander::isHighCostExpansionHelper(): bailout if no TTI is present to [SCEV] SCEVExpander::isHighCostExpansion(): assert if TTI is not provided.
lebedev.ri edited the summary of this revision. (Show Details)
mkazantsev accepted this revision.Feb 10 2020, 3:47 AM

Ok, with assert it makes sense. LGTM

This revision is now accepted and ready to land.Feb 10 2020, 3:47 AM

Ok, with assert it makes sense. LGTM

Great, thank you for the review!

Rebased, NFC.

Upload correct patch.

This revision was automatically updated to reflect the committed changes.