Page MenuHomePhabricator

NFC: Migrate SimplifyCFG to work on InstructionCost
ClosedPublic

Authored by sdesmalen on Jan 25 2021, 5:51 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:51 AM
sdesmalen requested review of this revision.Jan 25 2021, 5:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 25 2021, 5:51 AM
spatel added inline comments.Jan 26 2021, 10:49 AM
llvm/lib/Transforms/Utils/SimplifyCFG.cpp
338

Here and below: since we're updating function signatures, it would make sense to update the formatting as clang-tidy suggests:
computeSpeculationCost()

This can be done as an NFC preliminary commit.

364

Which patch defines InstructionBudget?

sdesmalen updated this revision to Diff 319659.Jan 27 2021, 1:49 PM
  • Use separate Cost and Budget variables in favour of InstructionBudget.
llvm/lib/Transforms/Utils/SimplifyCFG.cpp
338

That's a great suggestion, thanks!

364

This was initially added in D92238, but after some discussion on the patch I removed it in favour of using an incrementing Cost value that compares directly with a Budget value.

Nice. I think the separate cost and budget make this one easier to read. Address @spatel's issues then this looks good to me.

sdesmalen updated this revision to Diff 320154.Jan 29 2021, 9:40 AM

Split off the rename into a separate NFC patch and rebased this patch.
(I'll push the rename NFC patch together when this patch, when this one gets accepted)

spatel added inline comments.Jan 29 2021, 10:39 AM
llvm/lib/Transforms/Utils/SimplifyCFG.cpp
352–361

This code comment should be updated to reflect the new arguments/names.

sdesmalen updated this revision to Diff 320197.Jan 29 2021, 1:04 PM
sdesmalen marked an inline comment as done.

Updated comment.

llvm/lib/Transforms/Utils/SimplifyCFG.cpp
352–361

Good spot, thanks!

spatel accepted this revision.Jan 31 2021, 6:23 AM

LGTM - I have not followed all of the patches in this series, but this is mostly a direct translation of the existing code and improves readability.

This revision is now accepted and ready to land.Jan 31 2021, 6:23 AM

LGTM - I have not followed all of the patches in this series, but this is mostly a direct translation of the existing code and improves readability.

Thanks for the review/feedback!