Page MenuHomePhabricator

[CodeGen] Use delegate to notify targets when virtual registers are created
ClosedPublic

Authored by cdevadas on Sep 30 2022, 4:51 AM.

Details

Summary

This will help targets to customize certain codegen decisions based on
the virtual registers involved in special operations. This patch also
extends the existing delegate in MRI to start supporting multicast.

Diff Detail

Event Timeline

cdevadas created this revision.Sep 30 2022, 4:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2022, 4:51 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
cdevadas requested review of this revision.Sep 30 2022, 4:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2022, 4:51 AM

See the use case in D134951.
Testcase: llvm/test/CodeGen/AMDGPU/wwm-register-spill-during-regalloc.ll

qcolombet requested changes to this revision.Oct 5 2022, 10:14 AM

Hi,

Given this is completely target specific, have you considered implementing that in your target specific MachineFunctionInfo pass?
Also, could your use case be supported by the existing target flags that can be attached to MachineOperands?

If we were to go down adding target flags for virtual registers, there are a few things that we would need to consider like what do we do with the flags when we merge live-ranges (e.g., in the coalescer), could a flag become invalid (e.g., when we do some peephole), etc.

I feel this patch is creating the llvm equivalent of metadata with all the problems we have with them and I would rather we avoid that.

Cheers,
-Quentin

This revision now requires changes to proceed.Oct 5 2022, 10:14 AM

Hi,

Given this is completely target specific, have you considered implementing that in your target specific MachineFunctionInfo pass?
Also, could your use case be supported by the existing target flags that can be attached to MachineOperands?

MachineFunctionInfo isn't really a pass, it's a dumping ground for side data. The primary use of this is when new spill instructions are introduced, so there's no existing MachineOperand to attach this to. Plus a pass to re-add these implies the MIR was in an incorrect state and needed a fixup, which rarely works out well.

If we were to go down adding target flags for virtual registers, there are a few things that we would need to consider like what do we do with the flags when we merge live-ranges (e.g., in the coalescer), could a flag become invalid (e.g., when we do some peephole), etc.

Really what we want is for optimizations to not touch these at all. Coalescing shouldn't merge ranges with different flags.

I feel this patch is creating the llvm equivalent of metadata with all the problems we have with them and I would rather we avoid that.

The alternative is to make everything more aware of the cross lane nature of VGPRs, and I don't really know what that would look like

Really what we want is for optimizations to not touch these at all. Coalescing shouldn't merge ranges with different flags.

The problem is it is true for your use case, but it may not be true if other targets start using this.

Let me digest the information and come back to you.

The problem is it is true for your use case, but it may not be true if other targets start using this.

Let me digest the information and come back to you.

@qcolombet, much appreciate it if you could give feedback on the vreg flags.

much appreciate it if you could give feedback on the vreg flags.

I think that could be fine as long as we specifically say that they can be dropped inadvertently, i.e., you can't rely on them.

That said, I believe that if we put such language, you wouldn't be able to do what you want since in theory the information could be inaccurate and at this point, having this information or no information is pretty much the same. (I would actually argue that no information is probably better!)

much appreciate it if you could give feedback on the vreg flags.

I think that could be fine as long as we specifically say that they can be dropped inadvertently, i.e., you can't rely on them.

That said, I believe that if we put such language, you wouldn't be able to do what you want since in theory the information could be inaccurate and at this point, having this information or no information is pretty much the same. (I would actually argue that no information is probably better!)

They really can't be dropped. Missing information will be just as wrong. What we really want is to establish a dynamic value class. It's like a register class, except it can't map one to one with a physical register since in other contexts the same physical register could be used differently.

@qcolombet, you requested a new revision earlier. Let me know if there is any change still required before you can proceed with the review.

Hi all,

I've thought a little bit more about this.
Could we handle this with the existing delegate mechanism? (Look at how MRI_NoteNewVirtualRegister works.)

That way, you could do whatever target specific thing you want and it won't spill in the generic code.
To do that we may have to lift the limitation that we have only one delegate at a time (and we may need to add a few more hooks to notify the delegate on the events you are interested in.)

How does that sounds?

Cheers,
-Quentin

Hi all,

I've thought a little bit more about this.
Could we handle this with the existing delegate mechanism? (Look at how MRI_NoteNewVirtualRegister works.)

That way, you could do whatever target specific thing you want and it won't spill in the generic code.
To do that we may have to lift the limitation that we have only one delegate at a time (and we may need to add a few more hooks to notify the delegate on the events you are interested in.)

How does that sounds?

Cheers,
-Quentin

The delegate is not used in any target-specific passes so far. The specific cases they are currently used for, are looking reasonable. But I'm not really sure the delegate would be a good idea to handle the flags here. It seems like a more complicated way to propagate the flags.

Hi,

It seems like a more complicated way to propagate the flags.

It is definitely more complicated, but it doesn't bake any target specific knowledge in the generic code. In particular, in your case, you don't need any merging of the flags during coalescing and whatnot, but that may not be true for other targets if they want to adopt it and I'm not willing to open that door at the moment. Hence the suggested trade-off.

What I'm suggesting is:

  • Create your own pass that would attach your delegate to MRI before the point you need it.
  • Do whatever you want with your delegate.
  • Create another pass to unregister your delegate (you may even leave it there if it doesn't hurt anything.)

Cheers,
-Quentin

cdevadas updated this revision to Diff 470469.Oct 25 2022, 6:59 AM

Used the delegate method to notify targets when a new virtual register is created as well as cloned.

cdevadas retitled this revision from [CodeGen] Allow targets to define vreg flags to [CodeGen] Use delegate to notify targets when virtual registers are created.Oct 25 2022, 7:01 AM
cdevadas edited the summary of this revision. (Show Details)

I think we're going in the right direction, thanks for your patience with all this.

llvm/include/llvm/CodeGen/MachineRegisterInfo.h
68

I would go with something simpler: SmallPtrSet<Delegate *>.

160

I don't think we need that method and as a result we can ditch the DelegateReceiver altogether.

Essentially when an event occurs, we need to notify all the delegates.

If you still need a getter for your target specific use case, you can keep a hold on your custom delegate in your custom MachineFunctionInfo class.

Alternatively, I guess we could introduce the notion of ID (what you did with DelegateReceiver) but that must not be an enum. For instance, we could use something similar to Pass::ID for that, but for now if the MachineFunctionInfo solution works for you, let's not spend too much time on that.

For the record, the rationale for being against the enum is if people use more than one custom delegate, then the developer's experience is not great because we would have only one ID (RESERVED) to "register" it or they would need to modify the DelegateReceiver to add enum values.

163

After you ditch the DR, we would just need to check that the given delegate is already in the set.

168–170

Rename this in addDelegate and ditch the DelegateReceiver argument.

llvm/lib/CodeGen/MachineRegisterInfo.cpp
151

By moving this call here, the delegate may miss some important information (like the register class).
I wonder if it would make sense to send two "notifications": one when creating the incomplete register, like here, and one when the register is fully done (like what you did with the clone method).

Otherwise, we'll probably need to stick to having to send the notification just once where it originally was (i.e., within MachineRegisterInfo::createVirtualRegister, MachineRegisterInfo::createGenericVirtualRegister, and MachineRegisterInfo::cloneVirtualRegister).

What do you think?

175

We'll need to notify all delegates here, not just the RESERVED one.

cdevadas added inline comments.Oct 25 2022, 10:04 PM
llvm/include/llvm/CodeGen/MachineRegisterInfo.h
160

Will remove the enum class and can use just SmallPtrSet.

168–170

Sure

llvm/lib/CodeGen/MachineRegisterInfo.cpp
151

It makes sense to notify just like the earlier code. I will move it.

cdevadas updated this revision to Diff 470710.Oct 25 2022, 10:26 PM

Addressed review comments.

qcolombet accepted this revision.Oct 26 2022, 10:42 AM

LGTM, minor fixes inlined.

llvm/include/llvm/CodeGen/MachineRegisterInfo.h
61

Have the default implementation call MRI_NoteNewVirtualRegister and ditch the call to MRI_NoteNewVirtualRegister from cloneVirtualRegister.

61

Nit: I found ClonedReg to be ambiguous here: is it the result or the source of the cloning process?

Maybe use NewReg and SrcReg? (Ditto for noteCloneVirtualRegister)

llvm/lib/CodeGen/MachineRegisterInfo.cpp
175

Ditch this one (to make sure we don't double notify delegates that listen to both events.)

This revision is now accepted and ready to land.Oct 26 2022, 10:42 AM
cdevadas updated this revision to Diff 470900.Oct 26 2022, 12:38 PM

Thanks. Suggestions incorporated.

cdevadas updated this revision to Diff 471223.Oct 27 2022, 10:37 AM

Used auto * Delegate in the range-based loops.