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

[Juneyoung Lee via Phabricator via llvm-commits]

aqjune marked an inline comment as not done.
aqjune added inline comments.


================
Comment at: llvm/include/llvm/Target/Target.td:1067
+  let hasNoSchedulingInfo = 1;
+  let isNotDuplicable = 1;
+}
----------------
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.


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

https://reviews.llvm.org/D29014