[PATCH] D80249: CodeGen: Don't lazily construct MachineFunctionInfo

[Scott Linder via Phabricator via llvm-commits]

scott.linder added a comment.

In D80249#2158886 <https://reviews.llvm.org/D80249#2158886>, @nhaehnle wrote:

> In D80249#2156889 <https://reviews.llvm.org/D80249#2156889>, @arsenm wrote:
>
> > In D80249#2156869 <https://reviews.llvm.org/D80249#2156869>, @nhaehnle wrote:
> >
> > > Do you really need to change the signature of `MachineFunctionInfo::create`? That seems orthogonal to the problem you're trying to solve, or perhaps a redundant solution.
> > >
> > > The problem is: due to lazy construction and being able to refer to the MachineFunction, the state of the MachineFunctionInfo can have surprising accidental differences depending on when exactly it was created. There are two possible solutions:
> > >
> > > 1. Prevent the MachineFunctionInfo from accessing the MachineFunction.
> > > 2. Always create the MachineFunctionInfo in the same place.
> > >
> > >   This change does both, but either solution should be sufficient, and I personally find #1 counter-intuitive: it seems to me that the additional target-specific info about an object should be allowed to access that object.
> >
> >
> > You can't really do anything with the empty MachineFunction on construction. At the construction point, there isn't any information to access and it would always be a mistake to inspect it or make any construction decisions based on it (it would also be bad for the constructor to modify the MachineFunction).
>
>
> MFI is mutable, and so referring back to the MachineFunction as part of the mutation is potentially something reasonable to do. So the point isn't to use data from the MachineFunction during construction of the MFI, but rather to allow the "child" object to have a backlink to its "parent".


As in, the bug is for an MFI implementation to "cache" properties of the MF (e.g. AMDGPU taking a snapshot of MF properties in the MFI constructor), because the MF is mutable and there is no mechanism to invalidate the cache?


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

https://reviews.llvm.org/D80249