This is an archive of the discontinued LLVM Phabricator instance.

[SCEVExpander] NFCI: Change Cost type in costAndCollectOperands from int -> unsigned.
AbandonedPublic

Authored by sdesmalen on Nov 27 2020, 10:06 AM.

Details

Summary

This patch intends to be a non-functional change. It changes the
type of BudgetRemaining and Cost in costAndCollectOperands from 'int' to
'unsigned' so that the algorithm checks beforehand whether the resulting
budget is going to be exceeded or invalidated, rather than relying on
BudgetRemaining to end up < 0.

This refactoring is a preparation for InstructionCost added in D91174, which
has an Invalid state. By checking the 'invalidated' case early, InstructionCost
can be a drop-in replacement for unsigned when D91174 lands.

Diff Detail

Event Timeline

sdesmalen created this revision.Nov 27 2020, 10:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 27 2020, 10:06 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
lebedev.ri added a comment.EditedNov 27 2020, 10:19 AM

I don't like this.
I intentionally made this change from unsigned to signed back in D67318,
and then followed the same pattern when re-writing isHighCostExpansionHelper(),
because using signed is that much more easy to catch budget overrun
as opposed to basically ensuring that each subtraction doesn't cause an overflow
(or failing to do that, and having an infinite budget)

llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp
2345

Should we not add the test SubtractFromBudget(BudgetRemaining, Cost);
here too?
Because removing the test (BudgetRemaining<0) from here it may change the code path, and something that in the past could return true, now may return false because of the two tests below.

sdesmalen abandoned this revision.Dec 8 2020, 7:40 AM

I don't like this.
I intentionally made this change from unsigned to signed back in D67318,
and then followed the same pattern when re-writing isHighCostExpansionHelper(),
because using signed is that much more easy to catch budget overrun
as opposed to basically ensuring that each subtraction doesn't cause an overflow
(or failing to do that, and having an infinite budget)

Fair enough, I didn't realise it had already been changed before. I thought it would be useful to rewrite it in anticipation of the InstructionCost class, but I've rewritten that patch now (D92238).