[PATCH] D129086: [NFC][AMDGPU] Cleanup the SIOptimizeExecMasking pass.

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeExecMasking.cpp:744
+
+  Utils = std::make_unique<SIOptimizeExecMaskingUtils>(MF);
+
----------------
foad wrote:
> tsymalla wrote:
> > foad wrote:
> > > I don't understand the value of making this a separate class, instead of part of the main SIOptimizeExecMasking class.
> > After doing so and adding all necessary members to the SIOptimizeExecMasking class, it felt very cluttered.
> > I can revert that if you want, but the idea was to have a class that consists of various references to the sub-objects (TII, TRI etc.). These rely on the MachineFunction being passed. So, to ensure that these references are actually set, I created a separate class which can only be constructed by passing a MachineFunction. If we would add these members (references) to SIOptimizeExecMasking, we would be required to initialize them in the constructor AFAIK, which is not possible because of the required MachineFunction which is only passed in runOnMachineFunction. So there is the choice of using a) a separate class, b) using pointers and adding nullptr-checks everywhere or c) just go the unsafe route. I preferred to do a), cleanly abstract the sub-objects and all corresponding logic away, have the member references being initialized in the constructor of the Utils class.
> Even if you do want a separate class there is no need for dynamically creating an instance. You could just have a local variable:
> ```
>   SIOptimizeExecMaskingUtils Utils(MF);
>   bool Changed = Utils.optimizeExecSequence(MF);
>   Changed |= Utils.optimizeVCmpxAndSaveexecSequence(MF);
> ```
When you say "the unsafe route" you mean using pointers without adding nullptr checks? That is what I'd prefer, but I'm not going to insist. It looks like you are already doing that for ST in your utils class, which is a pointer with no nullptr checks.

If you are determine to keep the utils class I would prefer:
- Make the instance of the utils class a local variable in the SIOptimizeExecMasking::runOnMachineFunction method. Currently you dynamically create an instance which will survive after runOnMachineFunction returns, which feels wrong.
- Make MF a member variable of the utils class (as a reference not a pointer) so you don't have to keep passing it into the optimize* methods.
- Make ST a reference not a pointer.
- Perhaps replace the two optimize* methods with a single `bool run()` method.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129086