[PATCH] D23108: Implemented 132/213/231 forms selection for X86-FMA3-AVX512 opcodes.

[Vyacheslav Klochkov via llvm-commits]

v_klochkov added a comment.

Hi Craig,

Thank you for reviewing the patch.
I cancelled 2 code insertions accordingly to your recommendation, but did not do yet any changes in the FMA groups initialization.
Please see my comments below.

Thank you,
Vyacheslav


================
Comment at: llvm/lib/Target/X86/Utils/X86InstrFMA3Info.cpp:264
@@ +263,3 @@
+void X86InstrFMA3Info::initGroupsOnceImpl() {
+  FMA3_AVX2_FULL_GROUP(VFMADD);
+  FMA3_AVX2_FULL_GROUP(VFMSUB);
----------------
craig.topper wrote:
> These macros create a lot of small static tables and repeated initialization code for the map. Is there some way we can create fewer, larger static tables. Maybe a reg and mem table, a reg only table, and a mem only table with each row containing a group of opcodes and the attributes. Then loop through those larger tables to populate the map? Just a thought. Should reduce number of relocation entries and code size for this file.
> 
> Could probably be done as a follow up patch.
I don't see any really good ways to improve the initialization code.
If that is doable, then I suggest to have it as a follow up patch, that would only change the private fields/methods of the new classes without changing the public interfaces.

The requirements to the new classes:
- there should be some way to group 132/213/231 reg opcodes;
- to group 132/213/231 mem opcodes;
- to have mapping from reg opcodes to mem opcodes. Some future users also need mapping from mem to reg opcodes.
- to have quick mapping from reg-opcode to a group of 3 reg opcodes; have the same for mem opcodes.

================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:5961
@@ -6390,1 +5960,3 @@
     case X86::VFMSUB231SSr_Int: case X86::VFNMSUB231SSr_Int:
+    case X86::VFMADD132SSZr_Int: case X86::VFNMADD132SSZr_Int:
+    case X86::VFMADD213SSZr_Int: case X86::VFNMADD213SSZr_Int:
----------------
craig.topper wrote:
> This part isn't really related to the bigger change of introducing the new class and using it. And there are no test cases for this. Can this be split out with tests? Maybe find whatever test tests it for the VEX version and just add an AVX512 command line to it.
I agree and cancel this code insertion. Yes, it deserves a separate change-set.
I also think that the fix in this routine is still incomplete as it does not include support of some k-masked int opcodes.


https://reviews.llvm.org/D23108