[PATCH] D26855: New unsafe-fp-math implementation for X86 target

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added a reviewer: craig.topper.
RKSimon added a comment.

Some minor comments, but I'd like to hear other people's thoughts. This might be the first of a number of X86 MC patterns and I want to be sure we're doing this correctly.



================
Comment at: include/llvm/CodeGen/MachineInstr.h:1155
+  void print(raw_ostream &OS, ModuleSlotTracker &MST, bool SkipOpers = false,
+             const MachineBasicBlock *P = nullptr) const;
   void dump() const;
----------------
Explain the purpose of the new 'P' argument?


================
Comment at: lib/Target/X86/X86InstrInfo.cpp:9315
 
+static bool hasAllOnesOperand(MachineInstr &MI) {
+  auto Constants =
----------------
Rename this to hasExactlyOneOperand ? AllOnes means 'all bits are set'


================
Comment at: lib/Target/X86/X86InstrInfo.cpp:9344
+  // AllOnes value we'll use it
+  for (auto &MBB : MF) {
+    for (auto &MI : MBB) {
----------------
Rename to isDividendEactlyOne and update the comment.


================
Comment at: lib/Target/X86/X86InstrInfo.cpp:9415
+  MachineOperand Dividend = Root.getOperand(1);
+  unsigned DividendReg = Dividend.getReg();
+  bool DividendIsAllOnes = isDividendAllOnes(MF, DividendReg);
----------------
Dividend is only used once, merge these?

```
unsigned DividendReg = Root.getOperand(1).getReg();
```


================
Comment at: lib/Target/X86/X86InstrInfo.cpp:9416
+  unsigned DividendReg = Dividend.getReg();
+  bool DividendIsAllOnes = isDividendAllOnes(MF, DividendReg);
+
----------------
AllOnes -> ExactlyOne


================
Comment at: lib/Target/X86/X86InstrInfo.cpp:9481
+    if (MIDesc.getNumOperands() == 6)
+      LoadMI = BuildMI(MF, Root.getDebugLoc(), TII->get(Instrs[2]), LoadVReg)
+                   .addReg(PICBase)
----------------
Can we replace TII->get(Instrs[2]) with MIDesc?


================
Comment at: lib/Target/X86/X86InstrInfo.cpp:9488
+    else
+      LoadMI = BuildMI(MF, Root.getDebugLoc(), TII->get(Instrs[2]), LoadVReg)
+                   .addConstantPoolIndex(CPI);
----------------
Can we replace TII->get(Instrs[2]) with MIDesc?


================
Comment at: lib/Target/X86/X86InstrInfo.cpp:9600
+              X86::MULSSrr, X86::ADDSSrr, X86::MULSSrr,
+          },
+          Type::getFloatTy(MF.getFunction()->getContext()), Subtarget);
----------------
Inconsistent braces to the other cases.


https://reviews.llvm.org/D26855