Page MenuHomePhabricator

NFC: Migrate SpeculateAroundPHIs to work on InstructionCost
ClosedPublic

Authored by sdesmalen on Jan 25 2021, 5:56 AM.

Details

Summary

This patch migrates cost values and arithmetic to work on InstructionCost.
When the interfaces to TargetTransformInfo are changed, any InstructionCost
state will propagate naturally.

See this patch for the introduction of the type: https://reviews.llvm.org/D91174
See this thread for context: http://lists.llvm.org/pipermail/llvm-dev/2020-November/146408.html

Diff Detail

Event Timeline

sdesmalen created this revision.Jan 25 2021, 5:56 AM
sdesmalen requested review of this revision.Jan 25 2021, 5:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 25 2021, 5:56 AM

Aside from the one nit, this looks good to me.

llvm/lib/Transforms/Scalar/SpeculateAroundPHIs.cpp
329–330

I assume this assert is to keep this patch NFC? If so, please reword the message to imply that it should be possible for this to happen, but it's not implemented yet or add a FIXME comment.

sdesmalen added inline comments.Jan 27 2021, 1:48 PM
llvm/lib/Transforms/Scalar/SpeculateAroundPHIs.cpp
329–330

Not entirely; if a Constant cannot be folded when speculating, it must be possible to materialize it fully, and so we must be able to cost it. Otherwise there is something wrong with the existing IR. I don't think we should ever encounter this case.

ctetreau added inline comments.Jan 28 2021, 2:19 PM
llvm/lib/Transforms/Scalar/SpeculateAroundPHIs.cpp
329–330

If a user writes bad IR, the system shouldn't crash. If this is truly impossible except in cases of bad IR, then we should raise a fatal error.

sdesmalen added inline comments.Jan 29 2021, 9:30 AM
llvm/lib/Transforms/Scalar/SpeculateAroundPHIs.cpp
329–330

Perhaps I should have said: a constant should always be materializable and if it is materializable, it should also be costable. If that is not the case, the cost-model in the compiler is broken and needs fixing, not this pass.

If TotalMatCost is valid, then TotalFoldedCost must be valid as well, by the assert above.

ctetreau accepted this revision.Feb 1 2021, 9:54 AM

Aside from the nit, this looks good to me.

llvm/lib/Transforms/Scalar/SpeculateAroundPHIs.cpp
329–330

If TotalMatCost is valid, then TotalFoldedCost must be valid as well, by the assert above.

If TotalFoldedCost and TotalMatCost are both invalid, and both have the same value in the Value field, then an invalid TotalFoldingCost can survive the assert on line 329. It would assert on the next line, so in practice it wouldn't be an issue, but code changes could lead to issues.

Looking at the code, the comparators of InstructionCost seem to be messed up. operator== considers the Value field and the State field in all cases, but operator< does not consider the Value field if either the rhs or lhs are invalid. We should probably fix this.

a constant should always be materializable and if it is materializable, it should also be costable.

Then this assert should be above the other. This one states that constants can be materialized. The next one uses the cost of materialized constants.

Sorry if this all sounds super nitpicky. I'm just trying to understand the situation. I think reordering the asserts would help future readers understand the logic, but this is just a NIT.

This revision is now accepted and ready to land.Feb 1 2021, 9:54 AM
sdesmalen added inline comments.Feb 1 2021, 12:51 PM
llvm/lib/Transforms/Scalar/SpeculateAroundPHIs.cpp
329–330

Then this assert should be above the other. This one states that constants can be materialized. The next one uses the cost of materialized constants.

Good point, I'll reorder the two asserts before I commit the patch.

Sorry if this all sounds super nitpicky. I'm just trying to understand the situation.

No problem, I appreciate your attention to detail! I occasionally still try to get my head around what the right approach is, decisions like "assert vs fatal error vs bail out early" aren't always obvious. Any new insights I can use in the other InstructionCost patches.

This revision was landed with ongoing or failed builds.Feb 2 2021, 5:34 AM
This revision was automatically updated to reflect the committed changes.