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

[Vyacheslav Klochkov via llvm-commits]

v_klochkov added a comment.

Please accept my apologies for adding noise to the code-review process. 
Ok, no more cleaup-ups at the late code-review phases in future.

I could cancel the clean-up changes that I did, but I am not sure that will help to the review process now, i.e. when I already uploaded them.
If I should remove the clean-ups, no problems, I'll do that. Please let me know.

Regarding the '{' and '}' used for 'let' statements and removed from Nov05 patch...
Quentin, from you comment I did not understand if you disagree with removal of the braces.
Please let me know if the braces should be restored, I'll update the patch.

Thank you,
Slava


================
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,
----------------
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.

================
Comment at: llvm/lib/Target/X86/X86InstrFMA.td:57
@@ -56,3 +74,2 @@
 }
-} // Constraints = "$src1 = $dst"
 
----------------
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?



http://reviews.llvm.org/D13269