Page MenuHomePhabricator

[PowerPC] Modify the hasSideEffects of some VSX instructions from 1 to 0
ClosedPublic

Authored by ZhangKang on Dec 11 2019, 10:21 PM.

Details

Summary

If we didn't set the value for hasSideEffects bit in our td file, llvm-tblgen will set it as true for those instructions which has no match pattern.
Below 6 instructions don't set the hasSideEffects flag and don't have match pattern, so their hasSideEffects flag will be set true by llvm-tblgen.

But in fact below instructions don't modify any special register and don't have other SideEffects, they shouldn't have SideEffects.
This patch is to modify the hasSideEffects of below instructions from 1 to 0.

VEXTUHLX
VEXTUHRX
VEXTUWLX
VEXTUWRX
VSPLTBs
VSPLTHs

Diff Detail

Event Timeline

ZhangKang created this revision.Dec 11 2019, 10:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 11 2019, 10:21 PM

Modify the patch.

lkail accepted this revision.Dec 11 2019, 10:43 PM
lkail added a subscriber: lkail.

LGTM.

This revision is now accepted and ready to land.Dec 11 2019, 10:43 PM
Jim requested changes to this revision.Dec 11 2019, 11:59 PM
Jim added a subscriber: Jim.

It looks like not only this 6 instructions have to set hasSideEffects to 0.

llvm/lib/Target/PowerPC/PPCInstrAltivec.td
1381

hasSideEffects = 0 should be added class VX1_RT5_RA5_VB5.
Something like:

let hasSideEffects = 0 in
class VX1_RT5_RA5_VB5<bits<11> xo, string opc, list<dag> pattern>
...
This revision now requires changes to proceed.Dec 11 2019, 11:59 PM
In D71391#1781086, @Jim wrote:

It looks like not only this 6 instructions have to set hasSideEffects to 0.

Yes, in fact, I am checking the all instructions. The 6 VSX instructions will make the case failed. And I don't want to submit a huge patch, it's hard to review, so I submitted this patch.
The other instructions fixed will in another patch.

steven.zhang added inline comments.
llvm/lib/Target/PowerPC/PPCInstrAltivec.td
1381

I don't think it is good idea to clear this flag in the format class definition, except we have clear semantics that, there won't be any side effect for all the instructions with this format. But I don't see it, though, for now, it only have six instructions instance. What do you think ?

Jim added inline comments.Dec 12 2019, 12:46 AM
llvm/lib/Target/PowerPC/PPCInstrAltivec.td
1381

For now, It seems that VX1_RT5_RA5_VB5 is only for VEXTU* instructions. If any new instruction is added with this format, we can refactor the code for need.

lkail added a comment.EditedDec 12 2019, 1:04 AM

Hi @Jim we currently prefer to set this flag at a fine grained granularity to keep it accurate, it's the purpose of sending a series of patches to perform such cleanups.

ZhangKang marked an inline comment as done.Dec 12 2019, 1:11 AM
ZhangKang added inline comments.
llvm/lib/Target/PowerPC/PPCInstrAltivec.td
1381

Yes, I will update the patch to set the hasSideEffects in VX1_RT5_RA5_VB5 .

ZhangKang updated this revision to Diff 233529.Dec 12 2019, 1:12 AM

Set the hasSideEffects in VX1_RT5_RA5_VB5 .

Jim added a comment.Dec 12 2019, 1:25 AM

Hi @ZhangKang
You may follow @lkail 's suggestion is fine.
I am not a PowerPC guy. Just give some suggestions.

lkail added a comment.Dec 12 2019, 1:39 AM
In D71391#1781219, @Jim wrote:

Hi @ZhangKang
You may follow @lkail 's suggestion is fine.
I am not a PowerPC guy. Just give some suggestions.

Thanks @Jim !

steven.zhang added inline comments.Dec 16 2019, 6:34 PM
llvm/lib/Target/PowerPC/PPCInstrAltivec.td
1381

Set the flag for the instruction format is NOT right. Please move it to the instruction definition and run the benchmark. If everything goes well, we can go with this patch.

ZhangKang updated this revision to Diff 234513.Dec 18 2019, 4:42 AM

Set the hasSideEffect flag for on instructions instead for format.

ZhangKang marked an inline comment as done.Dec 18 2019, 4:43 AM
ZhangKang added inline comments.
llvm/lib/Target/PowerPC/PPCInstrAltivec.td
1381

Have updated the patch, I am testing the performance now.

ZhangKang marked an inline comment as done.Dec 20 2019, 1:01 AM

I have done the performance test for this patch, there is no degression.

llvm/lib/Target/PowerPC/PPCInstrAltivec.td
1381

I have done the performance test for this patch, there is no degression.

jsji added a comment.Dec 24 2019, 6:53 PM

@Jim Are you intend to block this by requesting changes? If so, can you have a look again? Thanks.

Jim accepted this revision.Dec 24 2019, 7:32 PM

LGTM.

This revision is now accepted and ready to land.Dec 24 2019, 7:32 PM
This revision was automatically updated to reflect the committed changes.