[llvm] r349167 - [globalisel][combiner] Make the CombinerChangeObserver a MachineFunction::Delegate

[Justin Bogner via llvm-commits]

Daniel Sanders via llvm-commits <llvm-commits at lists.llvm.org> writes:
> Author: dsanders
> Date: Fri Dec 14 09:50:14 2018
> New Revision: 349167
>
> URL: http://llvm.org/viewvc/llvm-project?rev=349167&view=rev
> Log:
> [globalisel][combiner] Make the CombinerChangeObserver a MachineFunction::Delegate
>
> Summary:
> This allows us to register it with the MachineFunction delegate and be
> notified automatically about erasure and creation of instructions. However,
> we still need explicit notification for modifications such as those caused
> by setReg() or replaceRegWith().
>
> There is a catch with this though. The notification for creation is
> delivered before any operands can be added. While appropriate for
> scheduling combiner work. This is unfortunate for debug output since an
> opcode by itself doesn't provide sufficient information on what happened.
> As a result, the work list remembers the instructions (when debug output is
> requested) and emits a more complete dump later.
>
> Another nit is that the MachineFunction::Delegate provides const pointers
> which is inconvenient since we want to use it to schedule future
> modification. To resolve this GISelWorkList now has an optional pointer to
> the MachineFunction which describes the scope of the work it is permitted
> to schedule. If a given MachineInstr* is in this function then it is
> permitted to schedule work to be performed on the MachineInstr's. An
> alternative to this would be to remove the const from the
> MachineFunction::Delegate interface, however delegates are not permitted
> to modify the MachineInstr's they receive.

Removing const seems like a much better alternative, IMO. More below.

> In addition to this, the observer has three interface changes.
> * erasedInstr() is now erasingInstr() to indicate it is about to be erased
>   but still exists at the moment.
> * changingInstr() and changedInstr() have been added to report changes
>   before and after they are made. This allows us to trace the changes
>   in the debug output.
> * As a convenience changingAllUsesOfReg() and
>   finishedChangingAllUsesOfReg() will report changingInstr() and
>   changedInstr() for each use of a given register. This is primarily useful
>   for changes caused by MachineRegisterInfo::replaceRegWith()
>
> With this in place, both combine rules have been updated to report their
> changes to the observer.
>
> Finally, make some cosmetic changes to the debug output and make Combiner
> and CombinerHelp
>
> Reviewers: aditya_nandakumar, bogner, volkan, rtereshin, javed.absar
>
> Reviewed By: aditya_nandakumar
>
> Subscribers: mgorny, rovka, kristof.beyls, llvm-commits
>
> Differential Revision: https://reviews.llvm.org/D52947
 ...
> Modified: llvm/trunk/include/llvm/CodeGen/GlobalISel/GISelWorkList.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/CodeGen/GlobalISel/GISelWorkList.h?rev=349167&r1=349166&r2=349167&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/CodeGen/GlobalISel/GISelWorkList.h (original)
> +++ llvm/trunk/include/llvm/CodeGen/GlobalISel/GISelWorkList.h Fri Dec 14 09:50:14 2018
> @@ -18,32 +18,60 @@ namespace llvm {
>
>  class MachineInstr;
>
> -// Worklist which mostly works similar to InstCombineWorkList, but on MachineInstrs.
> -// The main difference with something like a SetVector is that erasing an element doesn't
> -// move all elements over one place - instead just nulls out the element of the vector.
> -// FIXME: Does it make sense to factor out common code with the instcombinerWorkList?
> +// Worklist which mostly works similar to InstCombineWorkList, but on
> +// MachineInstrs. The main difference with something like a SetVector is that
> +// erasing an element doesn't move all elements over one place - instead just
> +// nulls out the element of the vector.
> +//
> +// This worklist operates on instructions within a particular function. This is
> +// important for acquiring the rights to modify/replace instructions a
> +// GISelChangeObserver reports as the observer doesn't have the right to make
> +// changes to the instructions it sees so we use our access to the
> +// MachineFunction to establish that it's ok to add a given instruction to the
> +// worklist.
> +//
> +// FIXME: Does it make sense to factor out common code with the
> +// instcombinerWorkList?
>  template<unsigned N>
>  class GISelWorkList {
> -  SmallVector<MachineInstr*, N> Worklist;
> -  DenseMap<MachineInstr*, unsigned> WorklistMap;
> +  MachineFunction *MF;
> +  SmallVector<MachineInstr *, N> Worklist;
> +  DenseMap<MachineInstr *, unsigned> WorklistMap;
>
>  public:
> -  GISelWorkList() = default;
> +  GISelWorkList(MachineFunction *MF) : MF(MF) {}
>
>    bool empty() const { return WorklistMap.empty(); }
>
>    unsigned size() const { return WorklistMap.size(); }
>
> -  /// Add - Add the specified instruction to the worklist if it isn't already
> -  /// in it.
> +  /// Add the specified instruction to the worklist if it isn't already in it.
>    void insert(MachineInstr *I) {
> -    if (WorklistMap.try_emplace(I, Worklist.size()).second) {
> -      Worklist.push_back(I);
> +    // It would be safe to add this instruction to the worklist regardless but
> +    // for consistency with the const version, check that the instruction we're
> +    // adding would have been accepted if we were given a const pointer instead.
> +    insert(const_cast<const MachineInstr *>(I));
> +  }
> +
> +  void insert(const MachineInstr *I) {
> +    // Confirm we'd be able to find the non-const pointer we want to schedule if
> +    // we wanted to. We have the right to schedule work that may modify any
> +    // instruction in MF.
> +    assert(I->getParent() && "Expected parent BB");
> +    assert(I->getParent()->getParent() && "Expected parent function");
> +    assert((!MF || I->getParent()->getParent() == MF) &&
> +           "Expected parent function to be current function or not given");
> +
> +    // But don't actually do the search since we can derive it from the const
> +    // pointer.
> +    MachineInstr *NonConstI = const_cast<MachineInstr *>(I);
> +    if (WorklistMap.try_emplace(NonConstI, Worklist.size()).second) {
> +      Worklist.push_back(NonConstI);

This all seems unnecessarily complicated, and I'm not a big fan of the
way it couples a simple worklist to the semantics of how it's used.

We're doing a bunch of work in the asserts here to prove that the "const
MachineInstr *" we've been given *isn't const*. Why provide an API that
claims to be const just to check at runtime that it wasn't const at all,
since if it was actually const this wouldn't work?

As far as I can tell the only reason to jump through these hoops is so
that the delegate API can be const, but I honestly don't see much value
in that - a comment that MF_HandleInsertion and MF_HandleDeletion
shouldn't modify the instruction in arbitrary ways is more than
sufficient.

I've attached an NFC patch that removes const from these interfaces.
WDYT?

-------------- next part --------------
A non-text attachment was scrubbed...
Name: gisel-wl-no-const.patch
Type: text/x-patch
Size: 11391 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20181214/31abb032/attachment.bin>
-------------- next part --------------

>      }
>    }
>
> -  /// Remove - remove I from the worklist if it exists.
> -  void remove(MachineInstr *I) {
> +  /// Remove I from the worklist if it exists.
> +  void remove(const MachineInstr *I) {
>      auto It = WorklistMap.find(I);
>      if (It == WorklistMap.end()) return; // Not in worklist.
>
>