[PATCH] D29014: [SelDag] Implement FREEZE node

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/FastISel.cpp:1573
+  // Lower Freeze to reg-reg copy.
+  unsigned Reg = getRegForValue(I->getOperand(0));
+  if (!Reg)
----------------
s/unsigned/Register


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:3109
+    EVT VT = Op.getValueType();
+    SmallVector<SDValue, 1> Values;
+    for (unsigned i = 0; i < Op.getNumOperands(); ++i) {
----------------
1 seems a bit small for the size


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:2238
+  // MachineInstr.
+  // LLVM translates an UNDEF node into multiple IMPLICIT_DEF
+  // instructions (in MachineInstr) if the UNDEF has multiple uses.
----------------
lebedev.ri wrote:
> mkuper wrote:
> > nlopes wrote:
> > > mkuper wrote:
> > > > I'm a bit confused about this. The function above selects UNDEF to IMPLICIT_DEF.
> > > > Where do we do the duplication? If we duplicate UNDEFs on the DAG level, then everything's already fine, and you can just select a FREEZE directly to IMPLICIT_DEF.
> > > > If we're duplicating IMPLICIT_DEFs, then this is a problem regardless of what you do here, if the freeze ends up being an IMPLICIT_DEF. Or are you saying we're duplicating IMPLICIT_DEFs in the DAG, but not in MI? I'd find that surprising.
> > > > 
> > > > (Note that the documentation for IMPLICIT_DEF just says that "this is the MachineInstr-level equivalent of undef." If we're not allowed to duplicate IMPLICIT_DEFs and they have a single defined value, that should be corrected.)
> > > If you have e.g.:
> > >  mul undef, undef
> > > 
> > > this eventually may get translated into:
> > >   %v1 = IMPLICIT_DEF
> > >   %v2 = IMPLICIT_DEF
> > >   %v3 = mul %v1, %v2
> > > 
> > > Even if it doesn't get translated to this explicitly, the semantics of UNDEF/IMPLICIT_DEF are that each read may see a different value.  For example, IMPLICIT_DEF can be sank into a loop and each iteration may see a different value, and known-bits analyses will also take advantage of the fact that each bit can take any value it wants.
> > > Therefore we use this "hack" of making a copy of the vreg where IMPLICIT_DEF is assigned to. This seems to guarantee that all uses see the same value, since there isn't a pass propagating IMPLICIT_DEF to copies.
> > If I understand correctly, the freeze.ll test shows you do end up with just an IMPLICIT_DEF eventually.
> > So I'm not sure where the benefit is? You rely on backend phase ordering to elide the copy only after the last time IMPLICIT_DEF may be duplicated? This seems a bit fishy.
> > 
> > Anyway, this is just a drive-by, I defer to Quentin and the other if they think this is reasonable.
> @qcolombet ping, please?
I see one potential problem with using a COPY of IMPLICIT_DEF. Suppose an instruction has two registers operands with different register classes. A codegen pass may reasonably try to rematerialize one of the implicit_defs to the natural operand register class to avoid the copy to satisfy operand constraints


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:2302
+  // Create a new virtual register.
+  unsigned NewVirtReg = RegInfo->createVirtualRegister(RC);
+  // Create CopyToReg node ('copy val into NewVirtReg')
----------------
s/unsigned/Register/


================
Comment at: test/CodeGen/X86/freeze.ll:89
+; X86ASM-NEXT:    retq
+  %y1 = freeze [2 x i64] undef
+  %v1 = extractvalue [2 x i64] %y1, 0
----------------
I'm not sure where Phabricator gets it syntax highlighting from, but it should add the freeze keyword


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D29014/new/

https://reviews.llvm.org/D29014