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

[Quentin Colombet via llvm-commits]

qcolombet added a comment.

Hi Slava,

> In this change-set I did not remove the old commuteInstruction() method as I am waiting for comments from you regarding my comment that says that this method can be removed only with the cost of duplication of the method code 5 times.


I think it makes sense to only expose one API and have one overridable internal API. Then put the boring code shared in the public function.
E.g., something like:
class stuff {
protected:

  virtual ty PublicAPIImpl(ty2);

public:

  /* not virtual */ ty PublicAPI(ty2) {
    /* boring code */
   return PublicAPIImpl(ty2);
  }

};

Thanks,
-Quentin


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:3499
@@ +3498,3 @@
+  if (OpcodeAlts[i].IsScalar && (SrcOpIdx1 == 1 || SrcOpIdx2 == 1))
+    return 0;
+
----------------
My understanding is that you are address this point here:
> (2) Fixed a correctness problem caused by commuting 1st and 2nd operands of scalar FMAs generated for intrinsics.

Most of the time I think we do not care about the high level bits of the value (which is what you are fixing here). Therefore, I wonder if we are not being pessimistic on the commutation opportunities.
I agree we should seek correctness first, but I wonder how often that high level setting is actually expected… We had this bug forever and apparently nobody noticed it. 

Anyway, what is your plan to get us the performance back?


http://reviews.llvm.org/D11370