[PATCH] D29014: [SelDag] Implement FREEZE node

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

@qcolombet - ping as per inline comment?
I'm also not really fully sold that the `SelectionDAGISel::Select_FREEZE()`
approach is fully future-proof/correct.

Other than that, nothing major stands out to me here.



================
Comment at: include/llvm/Analysis/TargetTransformInfoImpl.h:853
       return TTI::TCC_Free; // Model all PHI nodes as free.
 
     if (isa<ExtractValueInst>(U))
----------------
Also here
And in `TargetTransformInfo::getInstructionThroughput()`


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:4055
 
-
 SDValue DAGTypeLegalizer::PromoteIntRes_EXTRACT_SUBVECTOR(SDNode *N) {
----------------
Spurious newline change


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:3106
+  if (I.getOperand(0)->getType()->isAggregateType()) {
+    assert(Opcode == ISD::FREEZE);
+
----------------
`assert(Opcode == ISD::FREEZE && "Can only get here for `freeze` nodes");`


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:3110
+    SmallVector<SDValue, 1> Values;
+    for (unsigned i = 0; i < Op.getNumOperands(); ++i) {
+      SDValue Arg(Op.getNode(), i);
----------------
```
for (unsigned i = 0, NumOps = Op.getNumOperands(); i < NumOps; ++i) 
```


================
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.
----------------
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?


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

https://reviews.llvm.org/D29014