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

[Daniel Sanders via llvm-commits]

> On 14 Dec 2018, at 23:12, Justin Bogner <mail at justinbogner.com> wrote:
> 
> Daniel Sanders via llvm-commits <llvm-commits at lists.llvm.org <mailto: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 <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?

It's also checking that the instruction we've been notified about is really in the function. In DAGISel it's fairly common to create things and then throw them away without using them, relying on the garbage collection to delete them again. In GlobalISel that's wasteful both in terms of allocations and processing instructions that ultimately don't matter. It also leaks memory as there's no garbage collection until the whole function is deleted.

> 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?
> 
> <gisel-wl-no-const.patch>

Generally, I think it's a risky idea to rely on comments as a means to impose constraints on the code. It's better to enforce it with language features when they are available. It's easy to accidentally violate the constraint and spend days hunting for bugs, or completely miss that there is a bug (e.g. when processing instructions that aren't in the MF's MBB's). It's much better to have the language error out on the attempt to modify it.

For this particular case, I think it makes sense that the observer interface should take const ptrs since, in principle, observers observe rather than modify. If you start from that position then the question is about how we schedule work in the core algorithm in response to an observation. I originally took the view that GISelWorkList was responsible for the scheduling of work. From that point of view, the insert() method is a request to schedule work that it is already responsible for scheduling and as such it can take a const ptr and then use its own authority over the function to act on the request. This leads to the insert() that takes a const ptr, checking against the things it was created to work on, then de-consting and scheduling.

After a fairly lengthy debate with Aditya, it became clear that we disagree on whether GISelWorkList is responsible for scheduling the work. In his opinion it's the caller of insert() that's responsible for it which is an equally valid position. Adapting for that position, the observer interface is still const (because in the absence of additional factors it can only observe), however this particular observer implementation is part of the algorithm and therefore can own the scheduling of the work. It's therefore this observer implementation that decides whether the observation is something that impacts the worklist. The practical consequence is that the non-const MF reference and the check for whether the const MI is really in the non-const MF moves to the observer implementation

How does that sound?

>>     }
>>   }
>> 
>> -  /// 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.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20181217/13f7ff22/attachment.html>