GlobalISel: Add DstOp version of buildIntrinsic

[aditya_nandakumar]

The patch looks good. One comment - does it make sense to have two functions or unify the implementation - make the non DstOp version forward to DstOp version?

[arsenm]

In D61980#1504074, @aditya_nandakumar wrote:

The patch looks good. One comment - does it make sense to have two functions or unify the implementation - make the non DstOp version forward to DstOp version?

It would require creating another vector constructing DstOps, which seems worse

[aditya_nandakumar]

In D61980#1504092, @arsenm wrote:

In D61980#1504074, @aditya_nandakumar wrote:

The patch looks good. One comment - does it make sense to have two functions or unify the implementation - make the non DstOp version forward to DstOp version?

It would require creating another vector constructing DstOps, which seems worse

Fair enough. I'm not strongly opinionated here - either way is good for me.

[arsenm]

In D61980#1504116, @aditya_nandakumar wrote:

In D61980#1504092, @arsenm wrote:

In D61980#1504074, @aditya_nandakumar wrote:

The patch looks good. One comment - does it make sense to have two functions or unify the implementation - make the non DstOp version forward to DstOp version?

It would require creating another vector constructing DstOps, which seems worse

Fair enough. I'm not strongly opinionated here - either way is good for me.

There is some inconsistency in the current set of APIs. buildUnmerge provides ArrayRef<LLT> and ArrayRef<unsigned> variants, whereas buildInstr uses ArrayRef<DstOp>

[arsenm]

r360879

include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h

public:
   ///
   /// Callers are expected to add the required definitions and uses afterwards.
   ///
   /// \pre setBasicBlock or setMI must have been called.
   ///
   /// \return a MachineInstrBuilder for the newly created instruction.
   MachineInstrBuilder buildIntrinsic(Intrinsic::ID ID, ArrayRef<unsigned> Res,
                                      bool HasSideEffects);
+  MachineInstrBuilder buildIntrinsic(Intrinsic::ID ID, ArrayRef<DstOp> Res,
+                                     bool HasSideEffects);
 
   /// Build and insert \p Res = G_FPTRUNC \p Op
   ///
   /// G_FPTRUNC converts a floating-point value into one with a smaller type.
   ///
   /// \pre setBasicBlock or setMI must have been called.
   /// \pre \p Res must be a generic virtual register with scalar or vector type.
   /// \pre \p Op must be a generic virtual register with scalar or vector type.

lib/CodeGen/GlobalISel/MachineIRBuilder.cpp

auto MIB =
       buildInstr(HasSideEffects ? TargetOpcode::G_INTRINSIC_W_SIDE_EFFECTS
                                 : TargetOpcode::G_INTRINSIC);
   for (unsigned ResultReg : ResultRegs)
     MIB.addDef(ResultReg);
   MIB.addIntrinsicID(ID);
   return MIB;
 }
 
+MachineInstrBuilder MachineIRBuilder::buildIntrinsic(Intrinsic::ID ID,
+                                                     ArrayRef<DstOp> Results,
+                                                     bool HasSideEffects) {
+  auto MIB =
+      buildInstr(HasSideEffects ? TargetOpcode::G_INTRINSIC_W_SIDE_EFFECTS
+                                : TargetOpcode::G_INTRINSIC);
+  for (DstOp Result : Results)
+    Result.addDefToMIB(*getMRI(), MIB);
+  MIB.addIntrinsicID(ID);
+  return MIB;
+}
+
 MachineInstrBuilder MachineIRBuilder::buildTrunc(const DstOp &Res,
                                                  const SrcOp &Op) {
   return buildInstr(TargetOpcode::G_TRUNC, Res, Op);
 }
 
 MachineInstrBuilder MachineIRBuilder::buildFPTrunc(const DstOp &Res,
                                                    const SrcOp &Op) {
   return buildInstr(TargetOpcode::G_FPTRUNC, Res, Op);

unittests/CodeGen/GlobalISel/MachineIRBuilderTest.cpp

TEST_F(GISelMITest, TestBuildFPInsts) {
   ; CHECK: [[FSUB:%[0-9]+]]:_(s64) = G_FSUB [[COPY0]]:_, [[COPY1]]:_
   ; CHECK: [[FNEG:%[0-9]+]]:_(s64) = G_FNEG [[COPY0]]:_
   ; CHECK: [[FABS:%[0-9]+]]:_(s64) = G_FABS [[COPY0]]:_
   ; CHECK: [[FCOPYSIGN:%[0-9]+]]:_(s64) = G_FCOPYSIGN [[COPY0]]:_, [[COPY1]]:_
   )";
 
   EXPECT_TRUE(CheckMachineFunction(*MF, CheckStr)) << *MF;
 }
+
+TEST_F(GISelMITest, BuildIntrinsic) {
+  if (!TM)
+    return;
+
+  LLT S64 = LLT::scalar(64);
+  SmallVector<unsigned, 4> Copies;
+  collectCopies(Copies, MF);
+
+  // Make sure DstOp version works. sqrt is just a placeholder intrinsic.
+  B.buildIntrinsic(Intrinsic::sqrt, {S64}, false)
+    .addUse(Copies[0]);
+
+  // Make sure register version works
+  SmallVector<unsigned, 1> Results;
+  Results.push_back(MRI->createGenericVirtualRegister(S64));
+  B.buildIntrinsic(Intrinsic::sqrt, Results, false)
+    .addUse(Copies[1]);
+
+  auto CheckStr = R"(
+  ; CHECK: [[COPY0:%[0-9]+]]:_(s64) = COPY $x0
+  ; CHECK: [[COPY1:%[0-9]+]]:_(s64) = COPY $x1
+  ; CHECK: [[SQRT0:%[0-9]+]]:_(s64) = G_INTRINSIC intrinsic(@llvm.sqrt), [[COPY0]]:_
+  ; CHECK: [[SQRT1:%[0-9]+]]:_(s64) = G_INTRINSIC intrinsic(@llvm.sqrt), [[COPY1]]:_
+  )";
+
+  EXPECT_TRUE(CheckMachineFunction(*MF, CheckStr)) << *MF;
+}