[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:
> > > > 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. 


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

https://reviews.llvm.org/D29014