[GlobalISel] translate freeze to new generic G_FREEZE

[gargaroff]

Summary

As a follow up to https://reviews.llvm.org/D29014, add translation
support for freeze.

Introduce a new generic instruction G_FREEZE and translate freeze to it.

[gargaroff]

remove unrelated style changes from diff

[lebedev.ri]

This looks consistent with what we currently do in SelDAG, so LG in principle.

[aqjune]

LGTM in terms of semantic correctness.
Would this need another reviewer's acceptance to assure it's okay?

[fhahn]

Probably best to wait a bit before committing in case any GlobalIsel contributors have any thoughts.

[arsenm]

This doesn't match how the DAG handles it. This should use a dedicated G_FREEZE like the DAG, selected later

[aqjune]

I think it matches with what the current SelDAG does.
In the previous FREEZE patch the MachineInstr part didn't reach consensus, but SelDag part reached, so only the part was merged into master first. Currently it is lowered into MachineInstr's COPY.
I promised to make the MachineIR part in the future, but it didn't seem to be very high priority right now, so not made yet.

[aqjune]

The corresponding part in SelectionDAGISel looks like this:

void SelectionDAGISel::Select_FREEZE(SDNode *N) {
  // TODO: We don't have FREEZE pseudo-instruction in MachineInstr-level now.
  // If FREEZE instruction is added later, the code below must be changed as
  // well.
  CurDAG->SelectNodeTo(N, TargetOpcode::COPY, N->getValueType(0),
                       N->getOperand(0));
}

[arsenm]

In D77795#1972120, @aqjune wrote:

The corresponding part in SelectionDAGISel looks like this:

void SelectionDAGISel::Select_FREEZE(SDNode *N) {
  // TODO: We don't have FREEZE pseudo-instruction in MachineInstr-level now.
  // If FREEZE instruction is added later, the code below must be changed as
  // well.
  CurDAG->SelectNodeTo(N, TargetOpcode::COPY, N->getValueType(0),
                       N->getOperand(0));
}

This isn't equivalent. This is the selection of the intermediate ISD::FREEZE, not directly emitting a copy from the IR. The equivalent is here: https://github.com/llvm/llvm-project/blob/015dee1ac8988ed5184ef649c72ea0692f2633b3/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp#L10549

[arsenm]

The copy won't be legalized, so you're going to have issues emitting a copy for any non-legal source. We need a legalizable G_FREEZE

[gargaroff]

In D77795#1972248, @arsenm wrote:

The copy won't be legalized, so you're going to have issues emitting a copy for any non-legal source. We need a legalizable G_FREEZE

How should legalization rules look like? Since the value has to be stable I'm guessing zero extend for widen and unmerge/merge/extract/insert for narrow?

If we don't need to add selection support to any backend I could do that tomorrow.

My only concern is that the maintenance required to eventually introduce a new FREEZE MachineInstr which G_FREEZE should be selected to is quite a bit higher, since all GlobalISel backends that support G_FREEZE must be updated to properly select to FREEZE, instead of the COPY, while only having to change the IRTranslator is a lot easier.

But if legalization is a problem, then I guess there's nothing else that we can do.

[arsenm]

In D77795#1972303, @gargaroff wrote:

In D77795#1972248, @arsenm wrote:

The copy won't be legalized, so you're going to have issues emitting a copy for any non-legal source. We need a legalizable G_FREEZE

How should legalization rules look like? Since the value has to be stable I'm guessing zero extend for widen and unmerge/merge/extract/insert for narrow?

The same as whatever SelectionDAG does? It has the full set of legalize handling

[gargaroff]

In D77795#1972304, @arsenm wrote:

In D77795#1972303, @gargaroff wrote:

In D77795#1972248, @arsenm wrote:

The copy won't be legalized, so you're going to have issues emitting a copy for any non-legal source. We need a legalizable G_FREEZE

How should legalization rules look like? Since the value has to be stable I'm guessing zero extend for widen and unmerge/merge/extract/insert for narrow?

The same as whatever SelectionDAG does? It has the full set of legalize handling

Oh, right. Seems like I was totally getting ahead of myself 😅

Anyway. I'll try to come up with something tomorrow or over the weekend. Maybe just for scalars as a first step. If vectors turn out to not be too complex either, I'll try to add them as well.

[gargaroff]

introduce new generic instruction G_FREEZE

[gargaroff]

add missing check lines to legalizer-info-validation.mir

[arsenm]

llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp

2106–2107

Don't need llvm::

2110

Should add a dedicated buildFreeze

llvm/test/CodeGen/AArch64/GlobalISel/arm64-irtranslator.ll

2372

Can you add a struct freeze case? Also maybe use an argument instead of a constant?

[gargaroff]

remove unnecessary llvm::

add buildFreeze to MachineIRBuilder

add struct freeze test case

use argument instead of constant in test

[arsenm]

llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp

892 ↗

(On Diff #257696)

This can just go in the header

[gargaroff]

Seems like the pre-merge build bot is broken, since it's just showing me an HTML error instead of the build log. Everything should pass, so I went ahead and landed the change: 443c244cff6ac43735654b2d2c74ca06e7bab102

llvm/include/llvm/CodeGen/GlobalISel/IRTranslator.h

private:
 
   bool translateExtractElement(const User &U, MachineIRBuilder &MIRBuilder);
 
   bool translateShuffleVector(const User &U, MachineIRBuilder &MIRBuilder);
 
   bool translateAtomicCmpXchg(const User &U, MachineIRBuilder &MIRBuilder);
   bool translateAtomicRMW(const User &U, MachineIRBuilder &MIRBuilder);
   bool translateFence(const User &U, MachineIRBuilder &MIRBuilder);
+  bool translateFreeze(const User &U, MachineIRBuilder &MIRBuilder);
 
   // Stubs to keep the compiler happy while we implement the rest of the
   // translation.
   bool translateResume(const User &U, MachineIRBuilder &MIRBuilder) {
     return false;
   }
   bool translateCleanupRet(const User &U, MachineIRBuilder &MIRBuilder) {
     return false;
bool translateCatchPad(const User &U, MachineIRBuilder &MIRBuilder) {
     return false;
   }
   bool translateUserOp1(const User &U, MachineIRBuilder &MIRBuilder) {
     return false;
   }
   bool translateUserOp2(const User &U, MachineIRBuilder &MIRBuilder) {
     return false;
   }
-  bool translateFreeze(const User &U, MachineIRBuilder &MIRBuilder) {
-    return false;
-  }
 
   /// @}
 
   // Builder for machine instruction a la IRBuilder.
   // I.e., compared to regular MIBuilder, this one also inserts the instruction
   // in the current block, it can creates block, etc., basically a kind of
   // IRBuilder, but for Machine IR.
   // CSEMIRBuilder CurBuilder;

llvm/include/llvm/Support/TargetOpcodes.def

 
 /// Generic int to pointer conversion.
 HANDLE_TARGET_OPCODE(G_INTTOPTR)
 
 /// Generic bitcast. The source and destination types must be different, or a
 /// COPY is the relevant instruction.
 HANDLE_TARGET_OPCODE(G_BITCAST)
 
+/// Generic freeze.
+HANDLE_TARGET_OPCODE(G_FREEZE)
+
 /// INTRINSIC trunc intrinsic.
 HANDLE_TARGET_OPCODE(G_INTRINSIC_TRUNC)
 
 /// INTRINSIC round intrinsic.
 HANDLE_TARGET_OPCODE(G_INTRINSIC_ROUND)
 
 /// INTRINSIC readcyclecounter
 HANDLE_TARGET_OPCODE(G_READCYCLECOUNTER)

llvm/include/llvm/Target/GenericOpcodes.td

 }
 
 def G_DYN_STACKALLOC : GenericInstruction {
   let OutOperandList = (outs ptype0:$dst);
   let InOperandList = (ins type1:$size, i32imm:$align);
   let hasSideEffects = 1;
 }
 
+def G_FREEZE : GenericInstruction {
+  let OutOperandList = (outs type0:$dst);
+  let InOperandList = (ins type0:$src);
+  let hasSideEffects = 0;
+}
+
 //------------------------------------------------------------------------------
 // Binary ops.
 //------------------------------------------------------------------------------
 
 // Generic addition.
 def G_ADD : GenericInstruction {
   let OutOperandList = (outs type0:$dst);
   let InOperandList = (ins type0:$src1, type0:$src2);

llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp

 bool IRTranslator::translateFence(const User &U,
                                   MachineIRBuilder &MIRBuilder) {
   const FenceInst &Fence = cast<FenceInst>(U);
   MIRBuilder.buildFence(static_cast<unsigned>(Fence.getOrdering()),
                         Fence.getSyncScopeID());
   return true;
 }
 
+bool IRTranslator::translateFreeze(const llvm::User &U,
+                                   llvm::MachineIRBuilder &MIRBuilder) {
+  const Register DstReg = getOrCreateVReg(U);
+  const Register SrcReg = getOrCreateVReg(*U.getOperand(0));
+  MIRBuilder.buildInstr(TargetOpcode::G_FREEZE, {DstReg}, {SrcReg});
+  return true;
+}
+
 void IRTranslator::finishPendingPhis() {
 #ifndef NDEBUG
   DILocationVerifier Verifier;
   GISelObserverWrapper WrapperObserver(&Verifier);
   RAIIDelegateInstaller DelInstall(*MF, &WrapperObserver);
 #endif // ifndef NDEBUG
   for (auto &Phi : PendingPHIs) {
     const PHINode *PI = Phi.first;

llvm/test/CodeGen/AArch64/GlobalISel/arm64-irtranslator.ll

define i64 @test_readcyclecounter() {
   ; CHECK-LABEL: name:            test_readcyclecounter
   ; CHECK: [[RES:%[0-9]+]]:_(s64) = G_READCYCLECOUNTER{{$}}
   ; CHECK-NEXT: $x0 = COPY [[RES]]
   ; CHECK-NEXT: RET_ReallyLR implicit $x0
   %res = call i64 @llvm.readcyclecounter()
   ret i64 %res
 }
 
+define i64 @test_freeze() {
+  ; CHECK-LABEL: name:            test_freeze
+  ; CHECK: [[C:%[0-9]+]]:_(s64) = G_CONSTANT i64 0
+  ; CHECK-NEXT: [[RES:%[0-9]+]]:_(s64) = G_FREEZE [[C]]
+  ; CHECK-NEXT: $x0 = COPY [[RES]]
+  ; CHECK-NEXT: RET_ReallyLR implicit $x0
+  %res = freeze i64 0
+  ret i64 %res
+}
+
 !0 = !{ i64 0, i64 2 }

llvm/test/CodeGen/AArch64/GlobalISel/legalizer-info-validation.mir

 # DEBUG-NEXT: G_INTTOPTR (opcode {{[0-9]+}}): 2 type indices, 0 imm indices
 # DEBUG-NEXT: .. the first uncovered type index: 2, OK
 # DEBUG-NEXT: .. the first uncovered imm index: 0, OK
 #
 # DEBUG-NEXT: G_BITCAST (opcode {{[0-9]+}}): 2 type indices, 0 imm indices
 # DEBUG-NEXT: .. the first uncovered type index: 2, OK
 # DEBUG-NEXT: .. the first uncovered imm index: 0, OK
 #
+# DEBUG-NEXT: G_FREEZE (opcode {{[0-9]+}}): 1 type index, 0 imm indices
+# DEBUG-NEXT: .. type index coverage check SKIPPED: no rules defined
+# DEBUG-NEXT: .. imm index coverage check SKIPPED: no rules defined
+#
 # DEBUG-NEXT: G_INTRINSIC_TRUNC (opcode {{[0-9]+}}): 1 type index, 0 imm indices
 # DEBUG-NEXT: .. opcode {{[0-9]+}} is aliased to {{[0-9]+}}
 # DEBUG-NEXT: .. type index coverage check SKIPPED: user-defined predicate detected
 # DEBUG-NEXT: .. imm index coverage check SKIPPED: user-defined predicate detected
 # DEBUG-NEXT: G_INTRINSIC_ROUND (opcode {{[0-9]+}}): 1 type index, 0 imm indices
 # DEBUG-NEXT: .. opcode {{[0-9]+}} is aliased to {{[0-9]+}}
 # DEBUG-NEXT: .. type index coverage check SKIPPED: user-defined predicate detected
 # DEBUG-NEXT: .. imm index coverage check SKIPPED: user-defined predicate detected