This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Fold VPNOT into vpt blocks
Needs ReviewPublic

Authored by dmgreen on Aug 22 2019, 3:01 AM.

Details

Summary

We can fold a VPNOT into a predicate block, inverting the predicate to create "else" predicated instructions. This teaches the MVEVPTBlockPass to do that, updating the mask and instruction predicates of instructions following a VPNOT, and removing the VPNOT.

Diff Detail

Event Timeline

dmgreen created this revision.Aug 22 2019, 3:01 AM

I realised that this should probably be checking that the value produced by VPNOT is not used further along in the function. The VPT block "else" will not be updating the value like VPNOT will.

Would be good to see this rebased once D66577 lands.

llvm/lib/Target/ARM/MVEVPTBlockPass.cpp
116

nit: return mask % 2 ==1

dmgreen updated this revision to Diff 217459.Aug 27 2019, 11:51 AM

I had to make this check for Kill flags on VPNOT vpr regs, and it has gotten a bit of a rewrite in the process.

samparker added inline comments.Aug 29 2019, 6:28 AM
llvm/lib/Target/ARM/MVEVPTBlockPass.cpp
158

I'm struggling to understand the looping and the logic here. Why is a simple walk forward over the instructions not enough? I'd expect to see some conditions being checked and then just break when an instruction no longer meets the criteria. For instance, why can't the checks for kills and defs be performed in the do-loop above?

167

typos: backwoods and guarenteed.

dmgreen updated this revision to Diff 217904.Aug 29 2019, 9:00 AM
dmgreen marked 2 inline comments as done.
dmgreen added inline comments.
llvm/lib/Target/ARM/MVEVPTBlockPass.cpp
158

I was originally attempting to make it work something like that, but there was too much backtracking to make it work nicely. For example we were storing the LastMI and going back if an VPNOT was at the end of the block. Or if a VPNOT is found, we want to add it to the list of dead instructions, flip the CurrentModeT bit and inverse the "Then" to "Else" condition on all following instructions. Except if the value of VPR.P0 that the VPNOT produced is needed after the VPT block. Then we have to go back to before the last VPNOT, reversing all that we just did.

So instead, this is trying to keep it simpler. Checking what we want to change and then making the change.

The alternative, I think, would be replicating a lot of the code in the VPNOT forward scan, otherwise we would not know how far we had to look for dead vprs.

samparker added inline comments.Aug 30 2019, 1:58 AM
llvm/lib/Target/ARM/MVEVPTBlockPass.cpp
158

How about start by iterating backwards and only begin to add VPNOTs once you've found one that kills VPR? The scanning forward suggestions to me that it would be useful to know what's at the end first.

dmgreen marked an inline comment as done.Aug 30 2019, 8:23 AM
dmgreen added inline comments.
llvm/lib/Target/ARM/MVEVPTBlockPass.cpp
158

Hmm, I'm still not seeing that being simpler than this. If we scan backwards we wouldn't know where the block started! The current structure tries to get the start and end of the block, before doing the adjustments below.

Can you explain more about what you don't like about the current structure? I (very) initially had this getting a list of instructions in a vector (even though that didn't help much so I took it out). We may want in the future expand this to skip out non-predicate non-important instruction, to combine more into the same vpt block.

samparker added inline comments.Sep 2 2019, 3:03 AM
llvm/lib/Target/ARM/MVEVPTBlockPass.cpp
158

I don't like not being able to follow a small piece of code :) There's quite a lot of changes to iterators which is challenging my feeble mind, maybe just renaming them would help. But I guess that there's a few things that I don't understand:

  • Why do we need to know the start..? It looks like your search begins when you find something that isn't predicated, so why couldn't your search just halt there instead, if traversing backwards?
  • What is the first while loop doing on line 145? Why would there be multiple, consecutive, VPNOT instructions?
  • It looks like VPNOTs are counted against NumInstrs, but they'll be removed.

It turns out that I do really need this to stop the old version from crashing. But I think this patch is trying to do too much, both fixing the bugs and adding VPNOT's at the same time. I've re-done the fixes as D67219, and will get back to this later.

Can this be abandoned now?