[PATCH] D24447: [AVX-512] Add support for commuting VPTERNLOG.

[Vyacheslav Klochkov via llvm-commits]

v_klochkov accepted this revision.
v_klochkov added a comment.
This revision is now accepted and ready to land.

Hi Craig,

Excuse me for the delay, I did not realize that this patch was based on the existing FMA commute infrastructure
and thus I thought I was not the primary reviewer of this change-set.

Ok, I reviewed this patch and I have only 2 minor comments.
Otherwise, this patch looks great to me.

Regarding 'two address instruction pass' issue, I think it is another test case for this bug:
https://llvm.org/bugs/show_bug.cgi?id=17229
You may want to mention the problem with VTERNLOG in that bug.

Thank you,
Slava


================
Comment at: lib/Target/X86/X86InstrInfo.cpp:3324
@@ +3323,3 @@
+/// All prevents commuting a passthru operand. Returns -1 if the commute isn't
+/// possible.
+static int getThreeSrcCommuteCase(uint64_t TSFlags, unsigned SrcOpIdx1,
----------------
It may be good to mention here what the returned values 0, 1, 2 mean. I.e. 0 means that it is possible to commute operands SrcOp1 and SrcOp2, etc.

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:3341
@@ -3349,3 +3340,3 @@
     // elements of the first vector operand, for which the corresponding bit
     // in the k-mask operand is set to 0, are copied to the result of FMA.
     // TODO/FIXME: The commute still may be legal if it is known that the
----------------
"result of FMA."
The new function is not FMA specific. The comment section should be updated.


https://reviews.llvm.org/D24447