Page MenuHomePhabricator

[AMDGPU] Use AddedComplexity for ret and noret atomic ops selection
ClosedPublic

Authored by abinavpp on Jun 20 2022, 9:48 PM.

Details

Summary

This patch removes the predicate for return atomic ops and uses
AddedComplexity to distinguish its selection from its no return variant.
This will produce better matchers that doesn't unnecessarily check for
the negated predicate if the initial predicate failed. Also, it
simplifies the enabling of no return atomic ops selection in GlobalISel.

Diff Detail

Event Timeline

abinavpp created this revision.Jun 20 2022, 9:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 20 2022, 9:48 PM
abinavpp requested review of this revision.Jun 20 2022, 9:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 20 2022, 9:48 PM

This likely needs some tests?

This likely needs some tests?

We have tests for return and no return atomic ops. The goal here is to make sure those tests pass without modification.

arsenm added a comment.Jul 5 2022, 9:31 AM

Did the default complexity calculation account for the additional no use predicate in the opposite direction of what we want?

Did the default complexity calculation account for the additional no use predicate in the opposite direction of what we want?

I don't think I understood the question. I set the AddedComplexity in such a way that no-return matches are always done first since we're only adding predicates for those. By "default complexity", are we talking about getting PatternToMatch::getPatternComplexity() in CodeGenDagPatterns to give higher complexity to patterns with HasNoUse predicates?

arsenm added a comment.Jul 6 2022, 7:01 AM

I don't think I understood the question. I set the AddedComplexity in such a way that no-return matches are always done first since we're only adding predicates for those. By "default complexity", are we talking about getting PatternToMatch::getPatternComplexity() in CodeGenDagPatterns to give higher complexity to patterns with HasNoUse predicates?

Yes. I thought predicates boosted the complexity, such that this would work out by default:

// If this node has some predicate function that must match, it adds to the
// complexity of this node.
if (!P->getPredicateCalls().empty())
  ++Size;

I don't think I understood the question. I set the AddedComplexity in such a way that no-return matches are always done first since we're only adding predicates for those. By "default complexity", are we talking about getting PatternToMatch::getPatternComplexity() in CodeGenDagPatterns to give higher complexity to patterns with HasNoUse predicates?

Yes. I thought predicates boosted the complexity, such that this would work out by default:

// If this node has some predicate function that must match, it adds to the
// complexity of this node.
if (!P->getPredicateCalls().empty())
  ++Size;

Right, but I don't think we can rely on the the size of getPredicateCalls() here since (most of?) the return and no return atomic ops have one "predicate call" that does the address space check, memory-vt check and the no use check (if it's a no-return one). I'm not sure if there's a simple way to get the number of predicates specified in tablegen from TreePredicateFn other than calling each of its predicate checking functions (TreePredicateFn::isLoad(), TreePredicateFn::getMemoryVT() etc.).

arsenm accepted this revision.Jul 6 2022, 10:25 AM

LGTM although it seems like the default complexity logic could be smarter

This revision is now accepted and ready to land.Jul 6 2022, 10:25 AM

it seems like the default complexity logic could be smarter

I agree

This revision was landed with ongoing or failed builds.Jul 7 2022, 9:18 PM
This revision was automatically updated to reflect the committed changes.