[PATCH] D11370: Improved the interface of methods commuting operands, improved X86-FMA3 mem-folding&coalescing.

[Quentin Colombet]

qcolombet added a comment.

> So, the first alternative provides a flexible instrument that helps to know just what we need to know, while the second alternative makes findCommutaleOpIndices() to collect information that often is not needed later.


Right, but the first method does not tell the consumer that there are other alternatives for a given operand. The second method is much more general.

That being said, I believe you are right that most users of this method do not care about the alternatives, at least for now, so this is fine to have them iterate on the other indexes to check if there are alternatives. Exactly like you do in the two address pass.

Now, regarding the API, I would be in favor for something simpler, i.e.:

- Kill areOpsCommutable.
- Keep findCommutableOps with its current signature.

— Just make the two unsigned input/output parameters, like you did.
— Do not add two extra unsigned input parameters.


================
Comment at: llvm/lib/CodeGen/TargetInstrInfo.cpp:208
@@ +207,3 @@
+MachineInstr *TargetInstrInfo::commuteInstruction(MachineInstr *MI,
+                                                  bool NewMI) const {
+  unsigned OpIdx1 = ~0U, OpIdx2 = ~0U;
----------------
Shouldn’t we just need two default arguments ~0U, instead of duplicating the prototype?

================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:3546
@@ +3545,3 @@
+///
+bool X86InstrInfo::findCommutedOpIndices(MachineInstr *MI,
+                                         unsigned &SrcOpIdx1,
----------------
v_klochkov wrote:
> delena wrote:
> > I think this interface is inconvenient. I suggest to separate input and output. You can put ~0U as default value of input.
> Special thank you for this comment! Separating INPUT and OUTPUT arguments seems very reasonable. I like this idea.
> 
> In my opinion both approaches have right to live though. 
> Before adding 2 additional arguments to findCommutedOpIndices() and fixing other places I would wait for more comments from reviewers.
I do not see why it is better to separate the input and output parameters here.
As long as the parameter will have the value: CommuteAnyOperandIndex, we know how to make the distinction.


http://reviews.llvm.org/D11370