[PATCH] D29014: [SelDag] Implement FREEZE node

[Michael Kuperstein via Phabricator via llvm-commits]

mkuper added inline comments.


================
Comment at: include/llvm/CodeGen/ISDOpcodes.h:180
 
+    // FREEZE - FREEZE(VAL) returns a random integer if VAL is UNDEF (or
+    // is evaluated to UNDEF), or returns VAL otherwise. Note that each
----------------
random -> arbitrary (sorry, it's a pet peeve :-) )


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


https://reviews.llvm.org/D29014