This is an archive of the discontinued LLVM Phabricator instance.

CodeGen: Don't lazily construct MachineFunctionInfo
ClosedPublic

Authored by arsenm on May 19 2020, 2:39 PM.

Details

Summary

This fixes what I consider to be an API flaw I've tripped over
multiple times. The point this is constructed isn't well defined, so
depending on where this is first called, you can conclude different
information based on the MachineFunction. For example, the AMDGPU
implementation inspected the MachineFrameInfo on construction for the
stack objects and if the frame has calls. This kind of worked in
SelectionDAG which visited all allocas up front, but broke in
GlobalISel which hasn't visited any of the IR when arguments are
lowered.

I've run into similar problems before with the MIR parser and trying
to make use of other MachineFunction fields, so I think it's best to
just categorically disallow dependency on the MachineFunction state in
the constructor and to always construct this at the same time as the
MachineFunction itself.

A missing feature I still could use is a way to access an custom
analysis pass on the IR here.

I'll update the rest of the targets after initial comments

Diff Detail

Event Timeline

arsenm created this revision.May 19 2020, 2:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2020, 2:39 PM
ychen added a subscriber: ychen.May 19 2020, 5:24 PM

I'm not comfortable accepting this as I'm not familiar with the code, but it LGTM. The pattern of constructing the MachineFunctionInfo lazily on the first call to getInfo seems very non-obvious; it isn't implied by the name and it isn't described in the doc comment. As you note it is also particularly dangerous if it depends on values which are mutable over the life of the MachineFunction.

Is there no reasonable default MFI we can create?

Is there no reasonable default MFI we can create?

The point of it is to carry target specific information, so I don't think anything would make sense to be there by default.

I think there are a bunch of APIs that have this issue, so I think the general direction is good. I don't expect this to have any memory/compile-time impact, since probably even the smallest input will create the MachineFunctionInfo anyway. Was this the initial intention of having it build lazily?

So you should probably go ahead, I doubt anyone has any objections after all this time.

Nit: maybe commit the changes from AMDGPUMachineFunction.cpp separately

kparzysz added a comment.EditedJun 22 2020, 1:54 PM

A "create" function shouldn't return a null pointer. I think there should be a default MFI that has nothing in it, and it would be what the createMachineFunctionInfo returns a pointer to in the absence of overrides. It could even be statically allocated since is has no members:

virtual MachineFunctionInfo *
createMachineFunctionInfo(BumpPtrAllocator &Allocator, const Function &F,
                          const TargetSubtargetInfo *STI) const {
  static MachineFunctionInfo default;
  return &default;
}

Edit: fix indentation in the code

A "create" function shouldn't return a null pointer. I think there should be a default MFI that has nothing in it, and it would be what the createMachineFunctionInfo returns a pointer to in the absence of overrides. It could even be statically allocated since is has no members:

virtual MachineFunctionInfo *
createMachineFunctionInfo(BumpPtrAllocator &Allocator, const Function &F,
                          const TargetSubtargetInfo *STI) const {
  static MachineFunctionInfo default;
  return &default;
}

Edit: fix indentation in the code

Alternatively, make it pure virtual or make it llvm_unreachable?

arsenm updated this revision to Diff 274620.Jun 30 2020, 2:46 PM
arsenm retitled this revision from WIP: CodeGen: Don't lazily construct MachineFunctionInfo to CodeGen: Don't lazily construct MachineFunctionInfo.

Updated rest of the targets

A "create" function shouldn't return a null pointer. I think there should be a default MFI that has nothing in it, and it would be what the createMachineFunctionInfo returns a pointer to in the absence of overrides. It could even be statically allocated since is has no members:

virtual MachineFunctionInfo *
createMachineFunctionInfo(BumpPtrAllocator &Allocator, const Function &F,
                          const TargetSubtargetInfo *STI) const {
  static MachineFunctionInfo default;
  return &default;
}

Edit: fix indentation in the code

I did try to do this, but it didn't work out. It introduces a dependence on CodeGen from Target. I don't think anywhere else uses this pattern to avoid overrides, and just returns null. I don't think this is really an issue in practice, since this is only ever called from target code

A "create" function shouldn't return a null pointer. I think there should be a default MFI that has nothing in it, and it would be what the createMachineFunctionInfo returns a pointer to in the absence of overrides. It could even be statically allocated since is has no members:

virtual MachineFunctionInfo *
createMachineFunctionInfo(BumpPtrAllocator &Allocator, const Function &F,
                          const TargetSubtargetInfo *STI) const {
  static MachineFunctionInfo default;
  return &default;
}

Edit: fix indentation in the code

Alternatively, make it pure virtual or make it llvm_unreachable?

This is ok with me.

A "create" function shouldn't return a null pointer. I think there should be a default MFI that has nothing in it, and it would be what the createMachineFunctionInfo returns a pointer to in the absence of overrides. It could even be statically allocated since is has no members:

virtual MachineFunctionInfo *
createMachineFunctionInfo(BumpPtrAllocator &Allocator, const Function &F,
                          const TargetSubtargetInfo *STI) const {
  static MachineFunctionInfo default;
  return &default;
}

Edit: fix indentation in the code

Alternatively, make it pure virtual or make it llvm_unreachable?

This is ok with me.

As I mentioned in a previous comment, I think this ended up not working well. The dummy implementation I believe introduces build layer violations, and the pure virtual added more boilerplate to a couple of targets. Since this is only ever accessed from target code, I don't think there's much danger in using null here

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.

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.

What state in the MF is valid for the MFI to rely on at that well-defined point? If the MF is mutable, isn't the MFI potentially stale the moment the MF is mutated?

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

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

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?

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

But you can just as well pass in the machine function in these cases. I don't think the minor convenience of not passing in the function is worth the footgun this gives you

arsenm updated this revision to Diff 472813.Nov 2 2022, 5:33 PM

Rebase after once again running into issues caused by unclear construction time

Herald added a project: Restricted Project. · View Herald TranscriptNov 2 2022, 5:33 PM
arsenm added a comment.Nov 2 2022, 5:38 PM

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

MFI is always queried from the function itself, so there's no real context where you need the backlink. Having the MachineFunction in the constructor provides a huge footgun which I've had to fix the consequences of at least 5 different times. If you allow people to write their own quick and dirty machine analyses in the constructor, they will. Doing that at a random point causes confusion and hard to predict differences in target behavior.

thopre added a subscriber: thopre.Nov 6 2022, 1:51 AM

Considering

  1. Lazily constructing MFI is causing real pain, and makes it possible to write some silly bugs
  2. There haven't been any comments on this in a considerable amount of time, implying there aren't any strong opinions
  3. The drawback is that you have to type "MF" in some places

Right now, I think it would be good to just fix the existing footgun. I think it's probably okay to just land this?

kparzysz accepted this revision.Dec 7 2022, 3:23 PM
This revision is now accepted and ready to land.Dec 7 2022, 3:23 PM
arsenm updated this revision to Diff 483589.Dec 16 2022, 10:39 AM

Rebase. Move the construction of MFI out of the MachineFunction constructor to support the llvm-reduce usecase. It's slightly ugly to have to set this in the MachineModuleInfo and ResetMachineFunctionPass; the alternative might be a flag to skip constructing it

barannikov88 accepted this revision.Dec 16 2022, 12:21 PM
barannikov88 added a subscriber: barannikov88.

Few nitpicks and LGTM.
Ran into this once or twice as well

llvm/include/llvm/CodeGen/MachineFunction.h
104

Should STI be also passed by reference (can it be null)?

llvm/lib/Target/AArch64/AArch64MachineFunctionInfo.h
138–139

explicit is now redundant. (Here and in other backends.)

llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
683

It would be natural to pass AArch64FunctionInfo here instead of TargetSubtargetInfo. Like in AMDGPU backend below.

llvm/lib/Target/AMDGPU/AMDGPUMachineFunction.cpp
17

Calling constructor without arguments does not seem necessary...

llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.cpp
37

Passing GCNSubtarget instead of TargetSubtargetInfo would save a cast :)

arsenm updated this revision to Diff 484582.Dec 21 2022, 7:35 AM
arsenm marked 4 inline comments as done.

Avoid some casts

llvm/include/llvm/CodeGen/MachineFunction.h
104

getSubtargetImpl returns null by default, but not a whole lot of code bothers checking (e.g. the initial use when the function is constructed)

llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.cpp
37

Fixed with more templates