This is an archive of the discontinued LLVM Phabricator instance.

[ARM][MVE] Disallow VPSEL for tail predication
ClosedPublic

Authored by samparker on Jan 13 2020, 9:12 AM.

Details

Summary

Due to the current way that we collect predicated instructions, we can't easily handle vpsel in tail predicated loops. There are a couple of issues:

  1. It will use the VPR as a predicate operand, but doesn't have to be instead a VPT block, which means we can assert while building up the VPT block because we don't find another VPST to being a new one.
  2. VPSEL still requires a VPR operand even after tail predicating, which means we can't remove it unless there is another instruction, such as vcmp, that can provide the VPR def.

The first issue should be a relatively simple fix in the logic of the LowOverheadLoops pass, whereas the second will require us to represent the 'implicit' tail predication with an explicit value.

Diff Detail

Event Timeline

samparker created this revision.Jan 13 2020, 9:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 13 2020, 9:12 AM

Sounds good. I think that if we wanted to make VPSEL work, it would be better to try to convert them to VMOVt in a VPT block. (Although if there is only one of them, creating the block maybe more overhead. Hopefully the VCMP can be folded in).

Does VPNOT not need to be explicitly excluded because they are def's anyway? Might be worth leaving a comment to explain that to others (or us in the future). We might also want to say any instruction with an Else predicate (not a Then predicate) would be invalid too. But we don't generate those anywhere yet, so that can be separate.

There are a lot of large tests and it's hard to see what has changed and what hasn't with the patch. Can you add a quick comment to each that says what it's trying to test? And maybe if the MIR is modified. inloop-vpsel-1.mir and inloop-vpsel-3.mir seem to be the same.

llvm/test/CodeGen/Thumb2/LowOverheadLoops/inloop-vpnot-2.mir
225

This is a vpnot inside an VPT block? Crazy

This wasn't code-gen'd right? I don't think VPNOT of VPSEL should end up inside a VPT block at present. They are free-standing. (And VPNOT should turn the following instructions into "Else's", not "Then's".

Does VPNOT not need to be explicitly excluded because they are def's anyway?

It's actually the multiple vpr uses which prevent them from being included. I think I'll explicitly disallow them for now, as well as adding the comment.

Can you add a quick comment to each that says what it's trying to test? And maybe if the MIR is modified. inloop-vpsel-1.mir and inloop-vpsel-3.mir seem to be the same.

That they do! I will add comments and fix the test.

This wasn't code-gen'd right? I don't think VPNOT of VPSEL should end up inside a VPT block at present.

No, I just saw that I could predicate it so might as well.

samparker updated this revision to Diff 237907.Jan 14 2020, 3:05 AM
  • Added explicit check for VPNOT.
  • Added TODO comment.
  • Removed duplicate test.
  • Added comments to tests and had a small shuffle around of one of them.
dmgreen accepted this revision.Jan 14 2020, 3:33 AM

Thanks. LGTM

This revision is now accepted and ready to land.Jan 14 2020, 3:33 AM
This revision was automatically updated to reflect the committed changes.