Address code review feedback.

Author: maarquitos14

llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp

@@ -4013,7 +4013,6 @@ bool SPIRVInstructionSelector::selectModf(Register ResVReg,
   // independent elements of SPIRVResType. We can get each independent element
   // from I.getDefs() or I.getOperands().
   if (STI.canUseExtInstSet(SPIRV::InstructionSet::OpenCL_std)) {
-    uint32_t Opcode = CL::modf;
     MachineIRBuilder MIRBuilder(I);
     // Get pointer type for alloca variable.
     const SPIRVType *PtrType = GR.getOrCreateSPIRVPointerType(
@@ -4029,28 +4028,7 @@ bool SPIRVInstructionSelector::selectModf(Register ResVReg,
     // new register.
     GR.assignSPIRVTypeToVReg(PtrType, PtrTyReg, MIRBuilder.getMF());
     MachineBasicBlock &EntryBB = I.getMF()->front();
-    // At this point it's difficult to find the right position to insert the
-    // variable, because most instructions are still MachineInstruction and
-    // don't have SPIRV opcodes yet. OpFunction and OpFunctionParameter are
-    // already translated, so we will aim to insert the variable just after the
-    // last OpFunctionParameter, if any, or just after OpFunction otherwise.
-    auto VarPos = EntryBB.begin();
-    while (VarPos != EntryBB.end() &&
-           VarPos->getOpcode() != SPIRV::OpFunction) {
-      ++VarPos;
-    }
-    // Advance VarPos to the next instruction after OpFunction, it will either
-    // be an OpFunctionParameter, so that we can start the next loop, or the
-    // position to insert the OpVariable instruction.
-    ++VarPos;
-    while (VarPos != EntryBB.end() &&
-           VarPos->getOpcode() == SPIRV::OpFunctionParameter) {
-      ++VarPos;
-    }
-    // VarPos is now pointing at after the last OpFunctionParameter, if any,
-    // or after OpFunction, if no parameters.
-    // Create a new MachineInstruction for alloca variable in the
-    // entry block.
+    MachineBasicBlock::iterator VarPos = getPosForOpVariableWithinBlock(EntryBB);
     auto AllocaMIB =
         BuildMI(EntryBB, VarPos, I.getDebugLoc(), TII.get(SPIRV::OpVariable))
             .addDef(PtrTyReg)
@@ -4068,7 +4046,7 @@ bool SPIRVInstructionSelector::selectModf(Register ResVReg,
             .addDef(ResVReg)
             .addUse(GR.getSPIRVTypeID(ResType))
             .addImm(static_cast<uint32_t>(SPIRV::InstructionSet::OpenCL_std))
-            .addImm(Opcode)
+            .addImm(CL::modf)
             .setMIFlags(I.getFlags())
             .add(I.getOperand(3)) // Floating point value.
             .addUse(Variable);    // Pointer to integral part.

llvm/lib/Target/SPIRV/SPIRVUtils.cpp

@@ -995,4 +995,26 @@ unsigned getArrayComponentCount(const MachineRegisterInfo *MRI,
   return foldImm(ResType->getOperand(2), MRI);
 }
 
+MachineBasicBlock::iterator
+getPosForOpVariableWithinBlock(MachineBasicBlock &BB) {
+  // Find the position to insert the OpVariable instruction.
+  // We will insert it after the last OpFunctionParameter, if any, or
+  // after OpFunction otherwise.
+  MachineBasicBlock::iterator VarPos = BB.begin();
+  while (VarPos != BB.end() && VarPos->getOpcode() != SPIRV::OpFunction) {
+    ++VarPos;
+  }
+  // Advance VarPos to the next instruction after OpFunction, it will either
+  // be an OpFunctionParameter, so that we can start the next loop, or the
+  // position to insert the OpVariable instruction.
+  ++VarPos;
+  while (VarPos != BB.end() &&
+         VarPos->getOpcode() == SPIRV::OpFunctionParameter) {
+    ++VarPos;
+  }
+  // VarPos is now pointing at after the last OpFunctionParameter, if any,
+  // or after OpFunction, if no parameters.
+  return VarPos;
+}
+
 } // namespace llvm

llvm/lib/Target/SPIRV/SPIRVUtils.h

@@ -506,6 +506,8 @@ MachineInstr *getImm(const MachineOperand &MO, const MachineRegisterInfo *MRI);
 int64_t foldImm(const MachineOperand &MO, const MachineRegisterInfo *MRI);
 unsigned getArrayComponentCount(const MachineRegisterInfo *MRI,
                                 const MachineInstr *ResType);
+MachineBasicBlock::iterator
+getPosForOpVariableWithinBlock(MachineBasicBlock &BB);
 
 } // namespace llvm
 #endif // LLVM_LIB_TARGET_SPIRV_SPIRVUTILS_H

llvm/test/CodeGen/SPIRV/llvm-intrinsics/fp-intrinsics.ll

@@ -361,34 +361,24 @@ entry:
 
 define dso_local void @TestModf2(double noundef %d, ptr noundef %frac, ptr noundef %integral) {
 entry:
-  %d.addr = alloca double, align 4
-  %frac.addr = alloca ptr, align 8
-  %integral.addr = alloca ptr, align 8
-  store double %d, ptr %d.addr, align 4
-  store ptr %frac, ptr %frac.addr, align 8
-  store ptr %integral, ptr %integral.addr, align 8
-  %0 = load ptr, ptr %frac.addr, align 8
+  %0 = load ptr, ptr %frac, align 8
   %tobool = icmp ne ptr %0, null
   br i1 %tobool, label %lor.lhs.false, label %if.then
 
 lor.lhs.false:
-  %1 = load ptr, ptr %integral.addr, align 8
+  %1 = load ptr, ptr %integral, align 8
   %tobool1 = icmp ne ptr %1, null
   br i1 %tobool1, label %if.end, label %if.then
 
 if.then:
   br label %return
 
 if.end:
-  %2 = load ptr, ptr %frac.addr, align 8
-  %3 = load double, ptr %2, align 4
-  %4 = load ptr, ptr %integral.addr, align 8
-  %5 = load double, ptr %4, align 4
   %6 = tail call { double, double } @llvm.modf.f64(double %d)
   %7 = extractvalue { double, double } %6, 0
   %8 = extractvalue { double, double } %6, 1
   store double %7, ptr %frac, align 4
-  store double %7, ptr %integral, align 4
+  store double %8, ptr %integral, align 4
   br label %return
 
 return: