[PATCH] [X86] Memory folding for commutative instructions.

[Quentin Colombet]

Hi Simon

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:4218
@@ +4217,3 @@
+      if ((CommuteOpIdx1 == OriginalOpIdx) || (CommuteOpIdx2 == OriginalOpIdx)) {
+        MI = commuteInstruction(MI, false);
+        assert (MI && "commutable instruction failed to commute!");
----------------
I am not sure this is what we want generally speaking.
Indeed, if you look at the code you modified earlier (BTW having the full context would be easier for the review), we may not want to keep the commuted instruction unless the commute is in place:

MachineInstr *NewMI = commuteInstruction(MI, false);
// Unable to commute.
if (!NewMI) return 0;
if (NewMI != MI) { // <——— here if the instruction is different we do not commute.
  // New instruction. It doesn't need to be kept.
  NewMI->eraseFromParent();
  return 0;
}
Should we provide more arguments to have a finer control?

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:4219
@@ +4218,3 @@
+        MI = commuteInstruction(MI, false);
+        assert (MI && "commutable instruction failed to commute!");
+
----------------
This assert is not valid.
This is legal for commuteInstruction to return nullptr.
Make an early exit here.
See r208371 for more details.

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:4228
@@ +4227,3 @@
+        MI = commuteInstruction(MI, false);
+        assert (MI && "commutable instruction failed to commute!");
+
----------------
Ditto.

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:4218
@@ +4217,3 @@
+      if ((CommuteOpIdx1 == OriginalOpIdx) || (CommuteOpIdx2 == OriginalOpIdx)) {
+        MI = commuteInstruction(MI, false);
+        assert (MI && "commutable instruction failed to commute!");
----------------
I am not sure this is what we want generally speaking.
Indeed, if you look at the code you modified earlier (BTW having the full context would be easier for the review), we may not want to keep the commuted instruction unless the commute is in place:
      MachineInstr *NewMI = commuteInstruction(MI, false);
      // Unable to commute.
      if (!NewMI) return 0;
      if (NewMI != MI) { // <——— here if the instruction is different we do not commute.
        // New instruction. It doesn't need to be kept.
        NewMI->eraseFromParent();
        return 0;
      }

Should we provide more arguments to have a finer control?

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:4219
@@ +4218,3 @@
+        MI = commuteInstruction(MI, false);
+        assert (MI && "commutable instruction failed to commute!");
+
----------------
This assert is not valid.
This is legal for commuteInstruction to return nullptr.
Make an early exit here.
See r208371 for more details.

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:4228
@@ +4227,3 @@
+        MI = commuteInstruction(MI, false);
+        assert (MI && "commutable instruction failed to commute!");
+
----------------
Ditto.

http://reviews.llvm.org/D5701