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

[Tim Corringham via Phabricator via llvm-commits]

timcorringham marked 3 inline comments as done.
timcorringham 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;
----------------
arsenm wrote:
> It's not clear to me what this means, since every FP instruction does this
It is just a way of indicating to this pass that it is an instruction that uses the double precision flag.


================
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)) {
----------------
arsenm wrote:
> I'm not really comfortable inserting something semantically required at this point. Can you do this when the instructions are selected instead?
The problem with doing it during instruction selection is that we end up with many more mode register writes than are strictly required. As the mode register is not modelled as a register there isn't any way to track the values without a pass to do it, I suppose it is similar to adding nops or waitcnts, which are also done by specific passes.


================
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),
----------------
arsenm wrote:
> What does this mean exactly by "needs"? Does the instruction fail to function? 
The results can be outside the expected range when other rounding modes are used.


Repository:
  rL LLVM

https://reviews.llvm.org/D50633