[PATCH] D13269: Improved X86-FMA3 mem-folding & coalescing

[Quentin Colombet via llvm-commits]

qcolombet accepted this revision.
qcolombet added a comment.

Hi Slava,

Thanks for the clarifications.
And don’t worry about the “noise” part, I was just mentioning it for future reference :).

> from you comment I did not understand if you disagree with removal of the braces.


Heh, that’s the thing, I had mixed feeling about that, but now, I have made my mind and I am fine with the patch with the additional clean-ups you did.

Thanks for your patience and all your work on that!

Do you have commit access now or do you want me to commit on your behalf?

Cheers,
-Quentin


================
Comment at: llvm/lib/Target/X86/X86InstrFMA.td:37
@@ -19,1 +36,3 @@
+
+let Constraints = "$src1 = $dst", hasSideEffects = 0, isCommutable = 1 in
 multiclass fma3p_rm<bits<8> opc, string OpcodeStr,
----------------
v_klochkov wrote:
> qcolombet wrote:
> > Why are you changing the hasSideEffects semantic here?
> > I am not saying it is false, it just seems out of scope of what is necessary for this patch. What I am missing?
> If compare the patches from Nov05 and Oct05, then I just moved the setting of 'hasSideEffects = 0' from line 64 (Oct05) to here. That made the fma3p_forms multiclass more compact. It did not change semantics comparing to Oct05-patch.
> 
> Sorry if it added noise to the code-review process.
Oh, I was comparing from the initial source to the current update, i.e., unless I am mistaken there is a semantic change:
  defm r213 : fma3p_rm<opc213,
                       !strconcat(OpcodeStr, "213", PackTy),
                       MemFrag128, MemFrag256, OpTy128, OpTy256, Op>;
That being said, I believe the change is correct and I am fine with it being embedded in this patch.

================
Comment at: llvm/lib/Target/X86/X86InstrFMA.td:57
@@ -56,3 +74,2 @@
 }
-} // Constraints = "$src1 = $dst"
 
----------------
v_klochkov wrote:
> qcolombet wrote:
> > Although that is not incorrect, why do you get rid of that {?
> > You did the same later in that patch. Again, this is not wrong, but this kind of clean-ups adds noise to the review. I.e., if you want to do such clean-ups, which I am not sure I agree, that should be a separate patch.
> I am sorry for adding noise to the code-review process by doing clean ups in patches fixing already uploaded patches, i.e. like I did here. So, I shouldn't have removed 'IsMVariantCommutable' and 'IsRvariantCommutable'.
> 
> After I already removed those parameters, and set isCommutable = 1 at the line 18, I had to choose between: 
>   a) to update the line 57 (Oct05 rev):
>    "// Constraints = "$src1 = $dst" --> "// Constraints = "$src1 = $dst, isCommutable = 1"
> or
>   b) to just remove the line 57 and eliminate the need in updating it in future when/if the 'let' statement above is changed again.
> 
> Both variants required minor changes and I chose (b). Do you recommend restoring the '{' and '} // ....' used for 'let' statements?
> 
That’s fine, we can remove those.


http://reviews.llvm.org/D13269