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

[Vyacheslav Klochkov via llvm-commits]

v_klochkov added a comment.

Thank you for the answer, Quentin.

I agree with your new comments and started preparing an updated change-set.
Regarding the default ~0U values for arguments of commuteInstruction() method, I still need your opinion - please see my answer to your question ('Inline Comment') for details.

Summarizing the planned additional changes:

- remove areOpsCommutable()
- fix ~0U magic const
- change fixCommutedOpindices() to a helper method.
- ? remove old commuteInstruction() method and duplicate code handling ~0U args 5 times..., but ONLY if you recommend doing that.

I would also add that if eventually we need a method that would return ALL pairs of commutable operands,
then it could be a NEW method that would have the word 'All' in its name:
... findAllCommutedOpIndices();
Such method (if needed) should be done in a separate change-set though.

Thank you,
Slava


================
Comment at: llvm/include/llvm/Target/TargetInstrInfo.h:97-109
@@ -96,1 +96,15 @@
 
+  /// Assigns the (CommutableOpIdx1, CommutableOpIdx2) pair of commutable
+  /// operand indices to (ResultIdx1, ResultIdx2).
+  /// One or both input values of the pair: (ResultIdx1, ResultIdx2) may be
+  /// predefined to some indices or be undefined (designated by ~0U value).
+  /// The predefined result indices cannot be re-defined.
+  /// The function returns true iff after the result pair redefinition
+  /// the fixed result pair is equal to or equivalent to the source pair of
+  /// indices: (CommutableOpIdx1, CommutableOpIdx2). It is assumed here that
+  /// the pairs (x,y) and (y,x) are equivalent.
+  virtual bool fixCommutedOpIndices(unsigned &ResultIdx1,
+                                    unsigned &ResultIdx2,
+                                    unsigned CommutableOpIdx1,
+                                    unsigned CommutableOpIdx2) const;
+
----------------
MatzeB wrote:
> This is just a helper used to implement some of the other commuting methods. It should not be public and virtual, it doesn't even access any state and could be static or a helper function elsewhere.
Yes, I agree, good catch. This will be fixed.

================
Comment at: llvm/include/llvm/Target/TargetInstrInfo.h:100
@@ +99,3 @@
+  /// One or both input values of the pair: (ResultIdx1, ResultIdx2) may be
+  /// predefined to some indices or be undefined (designated by ~0U value).
+  /// The predefined result indices cannot be re-defined.
----------------
qcolombet wrote:
> Please define a static constant for this magic value.
> I do not like magic values wandering around without context.
> I do not have a good naming right now though, maybe UndefinedIndex?
I agree, we need a const for this value. How about 'CommuteAnyOperandIndex'
or 'AnyCommutableOperandIndex'?


================
Comment at: llvm/include/llvm/Target/TargetInstrInfo.h:274-277
@@ +273,6 @@
+  ///
+  /// The overloaded version of the method with the indices of the
+  /// commuted operands may be used when the commuted instruction has
+  /// more than two operands and thus, there may be preferences in what
+  /// operand must be commuted.
+  ///
----------------
MatzeB wrote:
> I think just adding Idx1,Idx2 to the existing method should be enough. The two operand commutable case will also work with that and it less confusing than having two overloaded variants around.
Matthias, can you please explain your idea? 
In particular, what do you mean by saying
     '...should be enough. The two operand commutable case will also work with that'?

I really want to just remove the old method that does not specify the commuted operands (i.e. to do exactly what you recommended here and to just add two operands to existing method). Unfortunately, I cannot do that without rewriting several places in LLVM.

This change-set replaces some calls of commuteInstruction(MI) calls with calls of commuteInstruction(MI,false,Idx1,Idx2). That made  the changes in those places more clear.

There are though 7 or 8 places where the old style method is called and rewriting those places would make this change-set significantly bigger.
For example, CodeGen/MachineCSE.cpp has 2 calls of old commuteInstruction() method.
That place is obviously has opportunities for improvement, but that should be done in a separate change-set as the current change-set is already very big.

So, the only reason for having two variants of commuteInstruction() method is the need to limit the size of the change-set, to limit the efforts needed for code-review, and to eliminate the risks (correctness, etc.) that are brought by too huge change-sets.

================
Comment at: llvm/lib/CodeGen/TargetInstrInfo.cpp:208
@@ +207,3 @@
+MachineInstr *TargetInstrInfo::commuteInstruction(MachineInstr *MI,
+                                                  bool NewMI) const {
+  unsigned OpIdx1 = ~0U, OpIdx2 = ~0U;
----------------
qcolombet wrote:
> Shouldn’t we just need two default arguments ~0U, instead of duplicating the prototype?
That could be done this way (i.e.  use ~0U value as default values for the indices).
This way is good as there will be only one commuteInstruction() method.

The only big disadvantage on that way is that I'll need to duplicate the code of this method 5 times:
1) in TargetInstrInfo.cpp (in commuteInstruction()
2) in X86 specific implementation of commuteInstruction() method
3) in PowerPC specific implementation
4) in AMDGPU specific implementation
5) in ARM specific implementation.

The duplicated code would be something like this:
    if (Idx1 == ~0U || Idx2 == ~0U) {
      if (!findCommutedOpIndices(MI, Idx1, Idx2)) {
        asssert(...);
        return nullptr;
      }
    }

After comparing the advantages and disadvantages... I would prefer to have this code in one place (i.e. as it is implemented now in the 1st change-set), but this question is not something very important for me 
and I will do as you recommend to do.



http://reviews.llvm.org/D11370