Page MenuHomePhabricator

Add "SkipDead" parameter to TargetInstrInfo::DefinesPredicate
ClosedPublic

Authored by NickGuy on Sep 29 2020, 8:21 AM.

Details

Summary

Some instructions may be removable through processes such as IfConversion,
however DefinesPredicate can not be made aware of when this should be considered.
This parameter allows DefinesPredicate to distinguish these removable instructions
on a per-call basis, allowing for more fine-grained control from processes like
ifConversion.

Renames DefinesPredicate to ClobbersPredicate, to better reflect it's purpose.

Diff Detail

Event Timeline

NickGuy created this revision.Sep 29 2020, 8:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 29 2020, 8:21 AM

I guess under this your classification, there are basically three possible kinds of predicable instruction:

  1. The instruction doesn't define a predicate
  2. The instruction defines a predicate, but the predicate is dead, and predicating it will make the definition go away.
  3. The instruction defines a predicate, and it will continue to define a predicate after predication.

If the boolean is specified, we check for 3, but not 2?

Do we really need the boolean to default to false? There are only two callers, as far as I can tell.

Can we rename the function to something like "ClobbersPredicate", if that's what we actually care about here?

I'm a little concerned IfConversion isn't really prepared to deal with the distinction here: in some cases, if conversion doesn't predicate all the instructions in the if-converted block.

For Thumb, would it ever make sense to undo size reduction if it would unblock if-conversion? (Off the top of my head, I guess this would only have an impact if we wanted to predicate something like an "adds" with a predicate that isn't dead, so maybe not worth investing any effort.)

Can we rename the function to something like "ClobbersPredicate", if that's what we actually care about here?

+1 from me. I also wonder renaming IncludesRemovable to something like Ignore/SkipDead?

Do we really need the boolean to default to false? There are only two callers, as far as I can tell.

It was done to reduce the footprint of this patch to be the interface change only, with any implementation changes in subsequent patches. I'll remove the default and hoist the call site changes up to this patch.

Can we rename the function to something like "ClobbersPredicate", if that's what we actually care about here?

I don't think that that's all we care about, unfortunately. A number of the backends this change effects check whether a predicate is defined or clobbered, while a few don't seem to have a concept of predicate clobbering (e.g. in R600InstrInfo).

For Thumb, would it ever make sense to undo size reduction if it would unblock if-conversion? (Off the top of my head, I guess this would only have an impact if we wanted to predicate something like an "adds" with a predicate that isn't dead, so maybe not worth investing any effort.)

Currently, the Thumb reduction pass is only invoked before IfConversion if we're optimising for size, so we'd prefer to keep the smaller size if possible (this change is an attempt to find a happy middle-ground, getting the performance gain from IfConversion while not sacrificing size)

I also wonder renaming IncludesRemovable to something like Ignore/SkipDead?

Done, will be included in the next patch

NickGuy updated this revision to Diff 299105.Oct 19 2020, 11:02 AM

I'm a little concerned IfConversion isn't really prepared to deal with the distinction here: in some cases, if conversion doesn't predicate all the instructions in the if-converted block.

I can't see any case where IfConversion would only predicate a portion of a block. The closest I can find is with common instructions and diamond conversion, but the common instructions are moved to the preceding block, and the IfConversion feasibility is then re-evaluated before any predication is performed.

efriedma accepted this revision.Oct 20 2020, 11:36 AM

LGTM

but the common instructions are moved to the preceding block, and the IfConversion feasibility is then re-evaluated before any predication is performed.

I don't think that's an accurate description of how it works... but I guess the conclusion is correct: instructions we plan to CSE are treated separately from instructions we plan to predicate.

This revision is now accepted and ready to land.Oct 20 2020, 11:36 AM
NickGuy retitled this revision from Add "IncludeRemovable" parameter to TargetInstrInfo::DefinesPredicate to Add "SkipDead" parameter to TargetInstrInfo::DefinesPredicate.Oct 21 2020, 2:24 AM
NickGuy edited the summary of this revision. (Show Details)
This revision was landed with ongoing or failed builds.Oct 21 2020, 3:52 AM
This revision was automatically updated to reflect the committed changes.
SjoerdMeijer added inline comments.Oct 21 2020, 4:10 AM
llvm/include/llvm/CodeGen/TargetInstrInfo.h
1402

A post-commit nit while I am catching up with this.
I found Eli's explanation very useful:

  1. The instruction doesn't define a predicate
  2. The instruction defines a predicate, but the predicate is dead, and predicating it will make the definition go away.
  3. The instruction defines a predicate, and it will continue to define a predicate after predication.

I think would be useful to add this to the comments, plus what SkipDead effects.
Don't think that would need a review.