Page MenuHomePhabricator

[ARM][MVE] Add invalidForTailPredication to TSFlags
ClosedPublic

Authored by samparker on Sep 11 2019, 6:36 AM.

Details

Summary

Set this bit for the MVE reduction instructions to prevent a loop from becoming tail predicated in their presence.

Diff Detail

Repository
rL LLVM

Event Timeline

samtebbs created this revision.Sep 11 2019, 6:36 AM
SjoerdMeijer added a subscriber: MatzeB.EditedSep 12 2019, 8:13 AM

At this point I am not sure if we should "pollute" the generic MC description with this property, mainly because the ARM backend will be the only user, and it looks like this could be simple target hook switching over the opcodes, like already done in the added unittest. Not pretty, but very simple and effective....
At the same time, there is enough prior art here, adding all sorts of things, so I am in two minds. But I think I would more lean towards a target hook, because it's not very generic/widely used.

Adding @MatzeB as a reviewer, who had some opinions on this in past if I remember that well.

At this point I am not sure if we should "pollute" the generic MC description with this property, mainly because the ARM backend will be the only user, and it looks like this could be simple target hook switching over the opcodes, like already done in the added unittest. Not pretty, but very simple and effective....
At the same time, there is enough prior art here, adding all sorts of things, so I am in two minds. But I think I would more lean towards a target hook, because it's not very generic/widely used.

I agree that this could be implemented solely in the backend. The problem I see, however, is what happens when adding new reduction instructions, as someone adding a new tablegen def won't know to add it to where these instructions are checked. Adding the flag to tablegen makes it easier to spot and add.

I agree that this could be implemented solely in the backend. The problem I see, however, is what happens when adding new reduction instructions, as someone adding a new tablegen def won't know to add it to where these instructions are checked. Adding the flag to tablegen makes it easier to spot and add.

Yes, I see that...but designing things for the future that may or may not happen at all is a bit tricky. As it's such a simple opcode check, I don't see why we should make things more complicated now for a thing that may never happen. And if it happens, it's not that we need to do a complete redesign of a tricky bit of logic. If it happens, then it will be pretty obvious from the codegen that new opcodes were not added to the TTI->isMVEReductionStmt hook, and then a decision can be made I guess.

Is there a way of writing to the TSFlags, of MCInstrDesc, via tablegen? Then we can get the nice markup and not have it affect the general flags. We could also do this later for our truncate and extend instructions too...

samparker commandeered this revision.Sep 16 2019, 1:32 AM
samparker edited reviewers, added: samtebbs; removed: samparker.
samparker updated this revision to Diff 220305.Sep 16 2019, 3:54 AM
samparker retitled this revision from [ARM] Add isVectorReduction MCInstrDesc flag to [ARM][MVE] Add invalidForTailPredication to TSFlags.
samparker edited the summary of this revision. (Show Details)

Ported to TSFlags. Removed and added opcode to the test.

SjoerdMeijer added a comment.EditedSep 16 2019, 4:00 AM

Nice one.

Bikeshedding: wondering if invalidForTailPredication is the same as not VPT Block Compatible, the term used in the ARM ARM....

They are different things. I actually don't think that the specification prevents any instructions from being in a TP loop, we just need to be careful of these. We should be able to allow most (all?) of these once we implement some better checks in the backend to look at the operands.

They are different things. I actually don't think that the specification prevents any instructions from being in a TP loop, we just need to be careful of these. We should be able to allow most (all?) of these once we implement some better checks in the backend to look at the operands.

Thanks, make sense, agreed.

unittests/Target/ARM/MachineInstrTest.cpp
16 ↗(On Diff #220305)

Does this TODO mean we are currently allowing instructions that should not be allowed? If so, is that a problem?

164 ↗(On Diff #220305)

Perhaps the message can be a little bit more informative what exactly is wrong here.

samparker marked 2 inline comments as done.Sep 16 2019, 5:19 AM
samparker added inline comments.
unittests/Target/ARM/MachineInstrTest.cpp
16 ↗(On Diff #220305)

We're not generating any TP loops yet, so we're okay! The approach will be to all the suspicious instructions here first so we don't have to test all of them when we finally implement the last part of predication.

SjoerdMeijer accepted this revision.Sep 16 2019, 5:26 AM

We're not generating any TP loops yet, so we're okay!

yes, that's true..... :-)

The approach will be to all the suspicious instructions here first so we don't have to test all of them when we finally implement the last part of predication.

but okay, that sounds like a little plan.

This looks good to me.

This revision is now accepted and ready to land.Sep 16 2019, 5:26 AM
samparker updated this revision to Diff 220315.Sep 16 2019, 5:28 AM

Improved error message.

Closed by commit rL372076: [ARM][MVE] Add invalidForTailPredication to TSFlags (authored by sam_parker, committed by ). · Explain WhySep 17 2019, 12:41 AM
This revision was automatically updated to reflect the committed changes.
danilaml added inline comments.
llvm/trunk/unittests/Target/ARM/CMakeLists.txt
2

Perhaps those need to be ${LLVM_MAIN_SRC_DIR} and ${LLVM_BINARY_DIR}, in case top-level CMake is in not in the llvm dir?