This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Refactor PPCInstrVSX.td
ClosedPublic

Authored by nemanjai on Apr 14 2020, 10:26 AM.

Details

Reviewers
hfinkel
jsji
Group Reviewers
Restricted Project
Commits
rG8ca2fc9993c9: [PowerPC] Refactor PPCInstrVSX.td
Summary

Over time, we have made many additions to this file and it has frankly become a bit of a mess. This has led to at least one issue - we have a number of instructions where the side effects flag should be set to false and we neglected to do this. This patch suggests a refactoring that should make the file much more maintainable. The file is split up into major sections and the nesting level is reduced, predicate blocks merged, etc.
Sections:

  • Custom PPCISD node definitions
  • Predicate definitions
  • Instruction formats
  • Instruction definitions
  • Helper DAG definitions
  • Anonymous patterns
  • Instruction aliases

Diff Detail

Event Timeline

nemanjai created this revision.Apr 14 2020, 10:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2020, 10:26 AM
jsji added a subscriber: jsji.Apr 14 2020, 1:45 PM

Overall is great, although I would prefer we don't touch hasSideEffects for floating point opcodes that might alter FPSCR. Thanks for refactoring!

Sections:
Custom PPCISD node definitions
Predicate definitions
Instruction formats
Instruction definitions
Helper DAG definitions
Anonymous patterns
Instruction aliases

Maybe we should add this summary to the header of file instead of in commit message only?

llvm/lib/Target/PowerPC/PPCInstrVSX.td
1240

xscmpoqp will alter CR field BF FPCC FX VXSNAN VXVC,
while we haven't modelled FPCC , VXSNAN etc in FPSCR for PowerPC.
Shouldn't we leave the hasSideEffects on for all similar opcodes?

1981

How do we intend to maintain this combinations list here? What order is this? Can we maintain a order that is easier to follow?

2008

Duplicated section? Why not merging them?

4403

Complexity is changed from 8 to 408 here, which is fine.

I like the way that this has been broken down into sections it will make our lives easier for future modifications.

llvm/lib/Target/PowerPC/PPCInstrVSX.td
1468

Do DFLOAD/DFSTORE need mayLoad / mayStore?
I know the original code did not have that so there may be a good reason for it.

1981

I do like the list idea since it is easier to see if something already exists.
However, I agree with Jinsong that we need a specific ordering otherwise we will be adding duplicates.
An idea for ordering:

All HasP8Vector
All HasP9Vector
All HasP9Altivec
All IsISAS3_0
Everything Else

It does not have to be this ordering but something to make duplicates easy to spot so that we only have to look at a small piece of the list in order to know if a section is there or not.

nemanjai marked 6 inline comments as done.Apr 21 2020, 3:57 AM

Overall is great, although I would prefer we don't touch hasSideEffects for floating point opcodes that might alter FPSCR. Thanks for refactoring!

I think we need to be a little more selective here. If we set that to all instructions that modify the FPSCR, that would mean it has to be set on all arithmetic, conversion/rounding and comparison operations. This would exclude all of them from being considered by LICM. Setting it on comparisons seems reasonable. But I don't think we should set it for anything that modifies the FPSCR - we can model whatever aspects need to be modeled rather than just throwing our hands up and saying "It has unmodeled side effects."

Maybe we should add this summary to the header of file instead of in commit message only?

Sounds good.

llvm/lib/Target/PowerPC/PPCInstrVSX.td
1240

I think it makes sense to set hasSideEffects on compare instructions.

1468

I agree 100% that those should be set. I am not sure how we missed these initially but I didn't want to change them in the refactoring patch.

1981

I attempted to order this chronologically (P7, P8, P9, etc.). However it may not be obvious here for two reasons:

  1. Endianness is orthogonal
  2. There are a few different predicates that really mean P9 (HasP9Vector, HasP9Altivec, IsISA3_0) and even P8 (HasP8Vector, HasDirectMove).

Maybe in the future, we end up getting rid of some of these predicates that mean the same thing and cannot really be separated out in real HW.

1981

I can perhaps add a comment describing what the order is.

2008

Just an omission. I tried to spot similar issues but I missed this one, I will merge them. Similarly with HasVSX, HasP9Altivec, IsLittleEndian.

4403

Yeah, I think it simplifies things to have everything that is defined in this file have the complexity boostl

jsji added a comment.Apr 21 2020, 8:07 AM

I think we need to be a little more selective here. If we set that to all instructions that modify the FPSCR, that would mean it has to be set on all arithmetic, conversion/rounding and comparison operations. This would exclude all of them from being considered by LICM. Setting it on comparisons seems reasonable. But I don't think we should set it for anything that modifies the FPSCR - we can model whatever aspects need to be modeled rather than just throwing our hands up and saying "It has unmodeled side effects."

Yes, agree. But before we model what needs to be modeled, maybe don't unset the flag in this refactor patch?

Yes, agree. But before we model what needs to be modeled, maybe don't unset the flag in this refactor patch?

Are you recommending that I ensure that the side effects flag remains set/unset exactly as it currently is with this refactor? I suppose I am fine with that as it separates out the patch to set the flag from the refactoring. I'll explicitly set the flag on instructions that currently have it set (along with a FIXME). Then I'll post a follow up patch that removes the explicitly set flag on instructions that I feel should not have the flag set. Is that acceptable?

nemanjai updated this revision to Diff 260264.Apr 27 2020, 3:21 AM
  • Reorder predicate blocks and describe the order at the start of the Anonymous Patterns section
  • Merge duplicate predicate blocks
  • Describe the file structure at the top of the file
  • Set hasSideEffects flag to match current behaviour along with a FIXME where the flag needed to be explicitly set

With the side effects flag set to match current behaviour, PPCGenInstrInfo.inc compares identical to what we currently get. As far as I can tell, PPCGenDAGISel.inc differs only in predicates and the reordering of a few patterns due to AddedComplexity being set on a few patterns where it hadn't been set before. Presumably all the patterns have LIT tests so the fact that the LIT test runs is clean is an indication that instruction selection is unchanged.

In a follow-up patch, I'll clean up the unnecessary side effects flag and add the missing mayLoad/mayStore flags on some of the load/store pseudos.

jsji accepted this revision as: jsji.Apr 27 2020, 7:50 AM

LGTM. Thanks for refactoring.

This revision is now accepted and ready to land.Apr 27 2020, 7:50 AM
This revision was automatically updated to reflect the committed changes.