[PATCH] D78132: [PowerPC] Refactor PPCInstrVSX.td

Tue Apr 21 04:17:39 PDT 2020

nemanjai marked 6 inline comments as done.
nemanjai added a comment.

In D78132#1981845 <https://reviews.llvm.org/D78132#1981845>, @jsji wrote:

> 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.

================
Comment at: llvm/lib/Target/PowerPC/PPCInstrVSX.td:1405
+  // QP Compare Ordered/Unordered
+  def XSCMPOQP : X_BF3_VA5_VB5<63, 132, "xscmpoqp", []>;
+  def XSCMPUQP : X_BF3_VA5_VB5<63, 644, "xscmpuqp", []>;
----------------
jsji wrote:
> `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?
> 
I think it makes sense to set `hasSideEffects` on compare instructions.

================
Comment at: llvm/lib/Target/PowerPC/PPCInstrVSX.td:1668
+                          "#DFSTOREf64",
+                          [(store f64:$XT, iaddrX4:$dst)]>;

----------------
stefanp wrote:
> Do DFLOAD/DFSTORE need mayLoad / mayStore?
> I know the original code did not have that so there may be a good reason for it.
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.

================
Comment at: llvm/lib/Target/PowerPC/PPCInstrVSX.td:2307
+//---------------------------- Anonymous Patterns ----------------------------//
+// Predicate combinations available:
+// [HasVSX]
----------------
stefanp wrote:
> jsji wrote:
> > How do we intend to maintain this `combinations` list here? What order is this?  Can we maintain a order that is easier to follow?
> 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.
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.

================
Comment at: llvm/lib/Target/PowerPC/PPCInstrVSX.td:2307
+//---------------------------- Anonymous Patterns ----------------------------//
+// Predicate combinations available:
+// [HasVSX]
----------------
nemanjai wrote:
> stefanp wrote:
> > jsji wrote:
> > > How do we intend to maintain this `combinations` list here? What order is this?  Can we maintain a order that is easier to follow?
> > 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.
> 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.
I can perhaps add a comment describing what the order is.

================
Comment at: llvm/lib/Target/PowerPC/PPCInstrVSX.td:2334
+// [HasVSX, HasP9Altivec]
+// [HasVSX, HasP9Altivec, IsBigEndian]
+// [HasVSX, HasP9Altivec, IsLittleEndian]
----------------
jsji wrote:
> Duplicated section? Why not merging them?
Just an omission. I tried to spot similar issues but I missed this one, I will merge them. Similarly with `HasVSX, HasP9Altivec, IsLittleEndian`.

================
Comment at: llvm/lib/Target/PowerPC/PPCInstrVSX.td:4349
+// Any Power9 VSX subtarget that supports Power9 Altivec.
+let Predicates = [HasVSX, HasP9Altivec] in {
+// Put this P9Altivec related definition here since it's possible to be
----------------
jsji wrote:
> Complexity is changed from 8 to 408 here, which is fine.
Yeah, I think it simplifies things to have everything that is defined in this file have the complexity boostl

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78132/new/

https://reviews.llvm.org/D78132