[PATCH] D50633: [AMDGPU] Add new Mode Register pass

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: lib/Target/AMDGPU/SIInstrFormats.td:124-126
+  // This bit indicates that this uses the floating point double precision
+  // rounding mode flags
+  field bit FPDPRounding = 0;
----------------
It's not clear to me what this means, since every FP instruction does this


================
Comment at: lib/Target/AMDGPU/SIModeRegister.cpp:160-180
+// Determine the Mode register setting required for this instruction.
+// Instructions which don't use the Mode register return a null Status.
+// Note this currently only deals with instructions that use the floating point
+// double precision setting.
+Status SIModeRegister::getInstructionMode(MachineInstr &MI,
+                                          const SIInstrInfo *TII) {
+  if (TII->usesFPDPRounding(MI)) {
----------------
I'm not really comfortable inserting something semantically required at this point. Can you do this when the instructions are selected instead?


================
Comment at: lib/Target/AMDGPU/SIModeRegister.cpp:171
+    case AMDGPU::V_INTERP_P2_F16:
+      // f16 interpolation instructions need double precision round to zero
+      return Status(FP_ROUND_MODE_DP(3),
----------------
What does this mean exactly by "needs"? Does the instruction fail to function? 


================
Comment at: lib/Target/AMDGPU/SIModeRegister.cpp:224
+  BlockData *NewInfo = new BlockData;
+  MachineInstr *InsertionPoint = 0;
+  bool RequirePending = true;
----------------
nullptr


================
Comment at: lib/Target/AMDGPU/SIModeRegister.cpp:347-348
+bool SIModeRegister::runOnMachineFunction(MachineFunction &MF) {
+  if (skipFunction(MF.getFunction()))
+    return false;
+
----------------
Since you seem to be relying on inserting these instructions, this is incorrect


================
Comment at: lib/Target/AMDGPU/SIModeRegister.cpp:367
+
+  // Phase 3 - add setregto the start of each block where the required entry
+  // mode is not satisfied by the exit mode of all its predecessors.
----------------
Typo setregto


Repository:
  rL LLVM

https://reviews.llvm.org/D50633