[PATCH] D86226: GlobalISel: Do not set observer of MachineIRBuilder in LegalizerHelper

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

In D86226#2250248 <https://reviews.llvm.org/D86226#2250248>, @aditya_nandakumar wrote:

> In D86226#2250210 <https://reviews.llvm.org/D86226#2250210>, @arsenm wrote:
>
>> In D86226#2250056 <https://reviews.llvm.org/D86226#2250056>, @aditya_nandakumar wrote:
>>
>>> My thought on this Observer ownership is as follows (which I think I mentioned in another review).
>>>
>>> - Each pass will setup an observer(wrapper) and install it to the MF. The MF at this point has the reference to the observer, and the pass owns each observer as it decides what it needs to observe and how.
>>> - At the end of each pass, just reset the observer (there are RAII helpers for this).
>>> - Remove the observer in MachineIRBuilder (is there a need for this?).
>>> - No need to thread observer through APIs. Instead we get it through `MF.getObserver()`.
>>>
>>> Thoughts?
>>
>> I'm mostly concerned with the wonky way we set register banks on new instructions with an observer. This isn't pass / function level, and is on an individual instruction we're legalizing.
>
> Sure, but what's the observer tracking in these cases  (I admit we don't use the regbankselect pass in our out of tree backend and I may be missing the pain points here).

It just grabs all the newly created / modified instructions, and then assigning register banks to the registers which haven't been set. This is manageable for the AMDGPU case where the individual opcodes don't really matter, but I would think other targets would struggle using it this way. I don't particularly like using the observer for this. It may be possible to have regbankselect iteratively work on the newly inserted instructions instead, but I haven't tried to do this.


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

https://reviews.llvm.org/D86226