This is an archive of the discontinued LLVM Phabricator instance.

[SimplifyCFG] Cost required selects
ClosedPublic

Authored by samparker on Jun 24 2020, 12:41 AM.

Details

Summary

Before we speculatively execute a basic block, query the cost of inserting the necessary select instructions against the phi folding threshold. For non-trivial insertions, a more accurate decision can probably be made during machine if-conversion.

Diff Detail

Event Timeline

samparker created this revision.Jun 24 2020, 12:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 24 2020, 12:41 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
RKSimon added inline comments.Jun 30 2020, 1:52 AM
llvm/test/Transforms/SimplifyCFG/ARM/select-costs.ll
87

Can you cleanup/reduce this test case any further?

lebedev.ri added inline comments.Jun 30 2020, 6:48 AM
llvm/lib/Transforms/Utils/SimplifyCFG.cpp
1994

I do not understand what is going on in this function.
Why are we comparing each separate cost with threshold,
instead of accumulating the costs and comparing that?

2016

if (CostOfSelects > PHINodeFoldingThreshold * TargetTransformInfo::TCC_Basic)

samparker marked an inline comment as done.Jun 30 2020, 7:59 AM
samparker added inline comments.
llvm/lib/Transforms/Utils/SimplifyCFG.cpp
1994

Hmm, I hadn't noticed that. Certainly happy to look into that next.

samparker updated this revision to Diff 274480.Jun 30 2020, 8:00 AM
  • Reduced test case further
  • Now multiplying cost by TTI::Basic.
fhahn added a subscriber: fhahn.Jul 13 2020, 8:25 AM
fhahn added inline comments.
llvm/test/Transforms/SimplifyCFG/ARM/select-costs.ll
4

I think it would be good to also add a triple for which we hoist the code, to ensure the test keeps testing what we want.

samparker updated this revision to Diff 277714.Jul 14 2020, 1:56 AM

Cheers, added a run for armv8a.

mkazantsev added inline comments.Jul 17 2020, 4:44 AM
llvm/test/Transforms/SimplifyCFG/ARM/select-costs.ll
1

I also don't quite get what this is about. I'd suggest the following:

  • Check in this test as is, without review, with current auto-generated checks;
  • Diff in patch will show what changed.
samparker updated this revision to Diff 278749.Jul 17 2020, 6:17 AM

Rebased after precommitting the test.

mkazantsev accepted this revision.Jul 28 2020, 5:15 AM

Looks good

This revision is now accepted and ready to land.Jul 28 2020, 5:15 AM
This revision was automatically updated to reflect the committed changes.

@mkazantsev I made a change before committing this, as I modified the patch that this depended upon. I've now added logic to query for CodeSize instead of SizeAndLatency when we have minsize as well as adding a minsize test so the logic is still tested.

lebedev.ri added a comment.EditedAug 21 2020, 5:50 AM

I guess it's an improvement for ARM, but the cost-modelling still looks bogus to me there.
It defies the point. We should calculate the single total cost of speculation,
and compare it once with the threshold, not compute parts of the cost
and compare them separately with the threshold each time.

llvm/lib/Transforms/Utils/SimplifyCFG.cpp
2107–2108

here too

2165–2170

and here

I agree and I'm just about to upload a couple more patches, one to cover the points you just raised.