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

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added a comment.

Still looking through this, but here are some initial comments.



================
Comment at: lib/Target/X86/X86InstrInfo.cpp:9196
+      MI->getParent()->getParent()->getConstantPool()->getConstants();
+  for (MachineInstr::mop_iterator MOI = MI->operands_begin(),
+                                  MOE = MI->operands_end();
----------------
ABataev wrote:
> I believe this can be replaced by a range-based loop
Use a range iterator if you can, same for the other cases below


================
Comment at: lib/Target/X86/X86InstrInfo.cpp:9206
+        if (auto *C = dyn_cast<ConstantFP>(ConstantEntry.Val.ConstVal))
+          if (C->isAllOnesValue())
+            return true;
----------------
This should be C->isExactlyValue(1.0)


================
Comment at: lib/Target/X86/X86InstrInfo.cpp:9214
+
+static bool isDividentAllOnes(MachineFunction &MF, unsigned DividentReg) {
+  // The divident could be AllOnes value and in this case we should not create
----------------
This should be isDividendExactlyOne. We should be calling it Dividend not Divident as well.


================
Comment at: lib/Target/X86/X86InstrInfo.cpp:9233
+        if (MO.getReg() == DividentReg) {
+          return hasAllOnesOperand(MI);
+        }
----------------
Embed hasAllOnesOperand here.


================
Comment at: lib/Target/X86/X86InstrInfo.cpp:9267
+                             DenseMap<unsigned, unsigned> &InstrIdxForVirtReg,
+                             SmallVector<int, 7> Instrs, int Iterations,
+                             Type *Ty, X86Subtarget &Subtarget) {
----------------
Replace SmallVector<int, 7> with ArrayRef<int>


================
Comment at: lib/Target/X86/X86InstrInfo.cpp:9381
+  // ResultMul
+  MachineInstrBuilder ResultMulMI =
+      BuildMI(MF, Root.getDebugLoc(), TII->get(Instrs[6]), ResultReg)
----------------
This isn't necessary if DividentIsAllOnes == true


https://reviews.llvm.org/D26855