Page MenuHomePhabricator

[ARM] Improve VPT predicate tracking
ClosedPublic

Authored by samparker on Sep 15 2020, 3:42 AM.

Details

Summary

The VPTBlock has been modified to track the 'global' state of the VPR, as well as the state for each block. Each object now just holds a list of instructions that makeup the block, while static structures hold the predicate information. This enables global access for querying how both a VPT block and individual instructions are predicated. These changes now allow us, again, to handle more complicated cases where multiple instructions build a predicate and/or where the same predicate in used in multiple blocks.
It doesn't, however, get us back to before the tracking was 'fixed' as some extra logic will be required to properly handle VPT instructions. Currently a VPT could be effectively predicated because of it's inputs, but the existing logic will not detect that and so will refuse to perform the transformation. This can be seen in remat-vctp.ll test where we still don't perform the transform.

Diff Detail

Event Timeline

samparker created this revision.Sep 15 2020, 3:42 AM
samparker requested review of this revision.Sep 15 2020, 3:42 AM
samparker updated this revision to Diff 292221.Sep 16 2020, 7:56 AM
  • Rebased.
  • Simplified ValidateMVEInst further by no longer differentiating between a main and secondary VCTPs.
SjoerdMeijer added inline comments.Sep 17 2020, 5:36 AM
llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp
168–169

Is this comment out of date? This class is not just representing a VPT block anymore as it e.g. contains member Blocks? A real proper nit in that case: perhaps VPTState or VPBlocks or something similar is a more accurate description of this class

194–197

Can you add a comment what this is? For example, at a first glance I was curious what the difference was with PredicatedInsts.

197

In one of the other patches, you had a nice comment explaining the chaining and AND'ing of predicates. Is that applicable here, would that comment be good to have here?

197

In one of the other patches, you had a nice comment explaining the chaining and AND'ing of predicates. Would it be good/applicable to have that comment here?
A proper nit in that case: CurrentPredicate -> CurrentPredicates?

1366

just checking, should this be <=?

samparker added inline comments.Sep 17 2020, 5:44 AM
llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp
197

Yeah, I had forgotten that there's already a comment describing it at the top of the file.

1366

No, that would be out of bounds.

samparker updated this revision to Diff 293376.Sep 22 2020, 1:26 AM
  • Rebased.
  • Renamed VPTBlock to VPTState.
  • Added a comment.
SjoerdMeijer accepted this revision.Sep 22 2020, 2:34 AM

Thanks, nice one.

This revision is now accepted and ready to land.Sep 22 2020, 2:34 AM
This revision was automatically updated to reflect the committed changes.