[PATCH] D29014: [SelDag][MIR] Add FREEZE

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/include/llvm/Target/Target.td:1067
+  let hasNoSchedulingInfo = 1;
+  let isNotDuplicable = 1;
+}
----------------
aqjune wrote:
> arsenm wrote:
> > aqjune wrote:
> > > arsenm wrote:
> > > > aqjune wrote:
> > > > > arsenm wrote:
> > > > > > arsenm wrote:
> > > > > > > aqjune wrote:
> > > > > > > > arsenm wrote:
> > > > > > > > > Why isNotDuplicable? I don't think this is the best maintained instruction property
> > > > > > > > It is because different freeze defs can yield different values. For example, consider following transformation:
> > > > > > > > ```
> > > > > > > > %undef = IMPLICIT_DEF
> > > > > > > > %x = FREEZE %undef
> > > > > > > > use(%x)
> > > > > > > > use(%x) // these two uses should see the same freezed value
> > > > > > > > ->
> > > > > > > > %undef = IMPLICIT_DEF
> > > > > > > > %x = FREEZE %undef
> > > > > > > > use(%x)
> > > > > > > > %x' = FREEZE %undef
> > > > > > > > use(%x') // It is possible that %x and %x' are assigned differently freezed values.
> > > > > > > > ```
> > > > > > > > This transformation is incorrect, because use()s after the transformation can see different values.
> > > > > > > > To prevent this class of optimizations, isNotDuplicable is set to 1.
> > > > > > > But these both are using the same undef vreg? The second freeze is still using the original undef, so this should be fine?
> > > > > > To clarify, I think the property you are really looking for is not the property given to you by isNotDuplicable. What you really care about is the input operand not using a new instance of undef. The "nonduplicability" is a function of the input value and not the instruction itself
> > > > > The second use of the undef register can be assigned a different physical register at (perhaps) register allocation, after seeing that its definition is IMPLICIT_DEF.
> > > > > ```
> > > > > %undef = IMPLICIT_DEF
> > > > > %x = FREEZE %undef
> > > > > use(%x)
> > > > > %x' = FREEZE %undef
> > > > > use(%x')
> > > > > =>
> > > > > %x = FREEZE $eax
> > > > > use(%x)
> > > > > %x' = FREEZE $ebx
> > > > > use(%x')
> > > > > ```
> > > > > This transformation can happen if there is a function call between them. `test/CodeGen/X86/freeze-call.ll` checks that it never happens.
> > > > > 
> > > > I think handling this correctly requires changing to how IMPLICIT_DEF  is handled in register allocation. I don't think noduplicate is really going to model this constraint sufficiently
> > > It would make complexity of this patch (lowering of freeze IR instruction) easier, but would the change in register allocation need performance tests?
> > > For example, if all uses of IMPLICIT_DEF should see a same caller-save register, the register should be spilled whenever its liveness crosses a function call.
> > > Currently IMPLICIT_DEF is commented as `MachineInstr-level equivalent of undef`, and undef`may evaluate to different values as well, so I wonder whether there are passes other than regalloc that use that semantics too.
> > > 
> > > isNotDuplicable's comment says
> > > 
> > > ```
> > > class MCInstrDesc {
> > > ...
> > >   /// Return true if this instruction cannot be safely
> > >   /// duplicated.  For example, if the instruction has a unique labels attached
> > >   /// to it, duplicating it would cause multiple definition errors.
> > >   bool isNotDuplicable() const { return Flags & (1ULL << MCID::NotDuplicable); }
> > > ```
> > > so I thought non-duplicability was a property of the freeze instruction itself.
> > I mean the definition of IMPLICIT_DEF needs to change to support this. I think we may actually need two different flavors of IMPLICIT_DEF. noduplicate is a stronger barrier to useful transforms, and also doesn't model the real problem here.
> > 
> > If you started out with two separate freeze instructions reading the same IMPLICIT_DEF register, you would still see the same problem in your example. 
> > I mean the definition of IMPLICIT_DEF needs to change to support this. I think we may actually need two different flavors of IMPLICIT_DEF.
> 
> Understood. So we can have two kinds of IMPLICIT_DEFs; the first one is an instruction that can be optimized to different values whenever used. The second one has the same value whenever it is used.
> 
> I think the second IMPLICIT_DEF is equivalent to FREEZE (IMPLICIT_DEF of the first kind), because freeze is an instruction that guarantees all uses of the value see the same value even though the operand is an undef-like value.
> 
> > If you started out with two separate freeze instructions reading the same IMPLICIT_DEF register, you would still see the same problem in your example.
> 
> If the program originally contained two separated freezes, then it is okay, because the program is originally written in that way. The programmer must have written down two freezes in LLVM IR bitcode, and they are lowered into MIR.
> The problematic case is when a program with a single freeze is optimized to a program with several freezes. Different freezes can return different values.
> ```
> %undef = IMPLICIT_DEF
> %x = FREEZE %undef
> use(%x)
> use(%x) // these two uses see the same value with help of freeze.
> ->
> %undef = IMPLICIT_DEF
> %x = FREEZE %undef
> use(%x)
> %x' = FREEZE %undef
> use(%x') // Now %x and %x' can be assigned different values, so this optimization is wrong.
> ```
I think it's important to define the rules here clearly for MIR, and not rely on expectations from the original IR. Some legalization for example may end up inserting multiple freezes of the same input value, and that should work correctly. Multiple freezes of the same input register should produce the same value. 


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

https://reviews.llvm.org/D29014