[PATCH] D61433: -Oz: Reuse constants in registers instead of canonicalizing operations that use a different constant

[Oliver Stannard (Linaro) via Phabricator via llvm-commits]

ostannard edited reviewers, added: ostannard; removed: olista01.
ostannard requested changes to this revision.
ostannard added inline comments.
This revision now requires changes to proceed.


================
Comment at: include/llvm/CodeGen/TargetInstrInfo.h:580
 
+  virtual unsigned changeAArch64Opcode(MachineInstr *MInstr) const {
+    return 0;
----------------
This name shouldn't mention AArch64 if it can be implemented for other targets, and shouldn't be here if it is AArch64 specific. It should also mention _what_ it does, not just "change".


================
Comment at: lib/CodeGen/MachineCSE.cpp:62
+  struct ConstMInst {
+    int OriginalValue = 0;
+    unsigned RegValue;
----------------
What if the value doesn't fit into an int (on the host system)?


================
Comment at: lib/CodeGen/MachineCSE.cpp:509
+// The above code uses the constant 655360000, once in a compare, once in a subtract.
+// AC6 will convert the subtract to an addition by a negative, which means
+// that it needs to materialise both the constant and the negative constant.
----------------
AC6?


================
Comment at: lib/CodeGen/MachineCSE.cpp:516
+// from Add to Sub.
+void MachineCSE::updateRegInfoAndInstr(MachineBasicBlock *MBB)
+{
----------------
This name tells me nothing about what the function does.


================
Comment at: lib/CodeGen/MachineCSE.cpp:523
+    if (MI->isMoveImmediate()) {
+      int ConstNum = MI->getOperand(1).getImm();
+      // iterate through all the constant values list in current function 
----------------
I don't think we can guarantee that operands 0 and 1 are used consistently across all targets.


================
Comment at: lib/CodeGen/MachineCSE.cpp:531
+          continue;
+        // serching current move instruction constant value is using in any where 
+        // in current function or constant value +1 or -1 or negative of that value.
----------------
If both positive and negative versions of the same immediate exist in the function, isn't this going to swap them both, and we'll still have both constants?

Why do we want to allow an offset of one? I don't see any code below which adjusts for this, so isn't this going to change the constant?


================
Comment at: lib/CodeGen/MachineCSE.cpp:559
+              MachineBasicBlock *MBBL = UseMI->getParent();
+              // if have any instruction is cmp inst in Machine Basic Block
+              // dont do this transformation.
----------------
Why? Also, this check will only detect global isel compare instructions, not target-specific ones.


================
Comment at: lib/CodeGen/MachineCSE.cpp:593
+              // then the instruction is add change to sub and vice versa.
+              if (TII->changeAArch64Opcode(UseMI)) {
+                BuildMI(*MBBL, InstIter, DL, TII->get(TargetOpc))
----------------
Duplicated call


================
Comment at: lib/CodeGen/MachineCSE.cpp:595
+                BuildMI(*MBBL, InstIter, DL, TII->get(TargetOpc))
+                        .add(UseMI->getOperand(0))
+                        .add(UseMI->getOperand(1))
----------------
Again, this is making assumptions about operands which might not be true for all targets.


================
Comment at: lib/CodeGen/MachineCSE.cpp:599
+                // replace the constant uses reg with top const num use reg value.
+                MRI->replaceRegWith(Reg, Newreg);
+                // erase the second move instruction from  parent basic block.
----------------
This is going to replace every use of the register, not just the one we have checked is safe.

I don't see any checks to make sure the add/sub instruction is dominated by the immediate move which we are re-using.


================
Comment at: lib/CodeGen/MachineCSE.cpp:623
+  if (MBB->getParent()->getFunction().optForSize()) {
+    updateRegInfoAndInstr(MBB); 
+  }
----------------
Why is this done here? It should be either properly integrated into the CSE algorithm, or made a separate pass.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61433/new/

https://reviews.llvm.org/D61433