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

[Jinsong Ji via Phabricator via llvm-commits]

jsji added a comment.

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?



================
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", []>;
----------------
`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?



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


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrVSX.td:2334
+// [HasVSX, HasP9Altivec]
+// [HasVSX, HasP9Altivec, IsBigEndian]
+// [HasVSX, HasP9Altivec, IsLittleEndian]
----------------
Duplicated section? Why not merging them?


================
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
----------------
Complexity is changed from 8 to 408 here, which is fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78132