Page MenuHomePhabricator

[ARM] Fix tail predication predicate tracking
ClosedPublic

Authored by samparker on Sep 14 2020, 7:08 AM.

Details

Summary

This patch mainly reorganising the logic in ValidateMVEInst because it was a bit difficult to follow... The important change is that we now clear the CurrentPredicate when we find an instruction which would completely overwrite the VPR. This fix essentially means we're back to not really being able to handle VPT instructions when tail predicating.

Diff Detail

Event Timeline

samparker created this revision.Sep 14 2020, 7:08 AM
samparker requested review of this revision.Sep 14 2020, 7:08 AM

I understood there is a NFC and non-NFC part of this patch. Is worth separating this out?

llvm/lib/Target/ARM/ARMInstrVFP.td
2849 ↗(On Diff #291566)

High-level question: is this worth to create a separate patch for this? Because we are adding MVEDomain, is this easily tested with assembly/disassembly tests?

2867 ↗(On Diff #291566)

This is redundant and we can just use the predicate on line 2848?

llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp
858

Can you clarify here if the "this means" applies more to the second point above, or to both?

860

a large VPT -> a large VPT block?

863

each the predication -> each predicate?

873

mve -> MVE

883

Perhaps just a nit about the message, but we are not really creating new VPT blocks here?

891

Beginning -> Beginning of?

899

Move this where it is used.

Cheers, I will definitely commit an NFC and rebase. I've extracted the validForTailPredication into another patch too: D87753.

samparker updated this revision to Diff 292172.Sep 16 2020, 3:49 AM

Removed all the unnecessary changes.

SjoerdMeijer accepted this revision.Sep 16 2020, 3:55 AM

Thanks for that. There was a lot going on before, but this now looks like a small, nice change.

This revision is now accepted and ready to land.Sep 16 2020, 3:55 AM
This revision was automatically updated to reflect the committed changes.