This is an archive of the discontinued LLVM Phabricator instance.

[globalisel][combiner] Make the CombinerChangeObserver a MachineFunction::Delegate
ClosedPublic

Authored by dsanders on Oct 5 2018, 2:16 PM.

Details

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.

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

Diff Detail

Event Timeline

dsanders created this revision.Oct 5 2018, 2:16 PM

Hi Daniel, the patch looks mostly good - I just have a couple of minor nitpicks.

include/llvm/CodeGen/GlobalISel/Combiner.h
49

It's not very clear from the comments regarding when the clients using the observer should call changingInstr vs changedInstr. What happens if when they are changing instruction, and they forget to call changinginst and only call changedInst?

include/llvm/CodeGen/GlobalISel/GISelWorkList.h
54

I understand what this is for, but I'm not sure if these asserts will ever trigger - and if we should have these.

include/llvm/CodeGen/GlobalISel/CombinerHelper.h
38

Also, should replaceRegWith respect register constraints (such as RegBank/RegClass) and emit copies while replacing?

dsanders added inline comments.Oct 15 2018, 3:03 PM
include/llvm/CodeGen/GlobalISel/Combiner.h
49

It's not an either-or thing, they should call both. There are two events involved in changing an instruction, the first (changingInstr) occurs before the changes are made and the second (changedInstr) occurs after the changes are made.

include/llvm/CodeGen/GlobalISel/CombinerHelper.h
38

It's not necessary for the calls in CombineHelper right now but we will certainly want a variant of this that does that at some point

include/llvm/CodeGen/GlobalISel/GISelWorkList.h
54

Assuming we write our code correctly, they won't trigger :-).

We spoke off-list about this and it's my understanding that your main concern is that GISelWorkList should be able to handle instructions that aren't in a function+bb. We don't have any such instances right now but I see your point. We could potentially tweak the name to make it clearer or we could wait until we have a case that works on MI's that aren't in a block/function and resolve it then. I suspect we're unlikely to find such a case in the near future since it's not common to create an instruction but keep it outside of bbs/functions long term.

lib/CodeGen/GlobalISel/Combiner.cpp
65

This line shouldn't be here and is probably the cause of your confusion about the difference between changingInstr() and changedInstr(). It won't cause any trouble but it's not necessary.

include/llvm/CodeGen/GlobalISel/CombinerHelper.h
38

It would be great if you can address this (even with a naive check if constraints are not met insert a copy). This can be a very subtle bug to figure out when it starts happening.

include/llvm/CodeGen/GlobalISel/GISelWorkList.h
54

Yes. Please update the comment to reflect this new change. Also if you're checking that you're inserting valid instructions (with parents), you should move these asserts to the non const method as well.

dsanders updated this revision to Diff 175776.Nov 28 2018, 2:55 PM

Obey constraints in replaceRegWith()
Correct the extending load combine use of replaceRegWith where we replace the
input of an extend to a wider-than-preferred type. Previously we were calling
replaceRegWith() multiple times for the same reg and changing the types of the
vreg in the process. We'd always fix it by the time we were finished but we have
asserts checking the intermediates now.

dsanders marked 6 inline comments as done.Nov 28 2018, 3:03 PM
dsanders added inline comments.
include/llvm/CodeGen/GlobalISel/GISelWorkList.h
54

I believe the only remaining review comment is this one. I'm not sure what change you're referring to here w.r.t the comment to be updated (probably because it's taken so long for me to get back to this, sorry).

The asserts are only necessary where it's not clear that you have the right to modify the instructions which is only an issue for the const version. The non-const version is clearly allowed to modify it as it's not const whereas the const version has to know it can find a non-const pointer to the same object.

dsanders updated this revision to Diff 178043.Dec 13 2018, 5:59 AM

Rebase and fix the remaining issue about documenting the right-to-modify
MachineInstrs within the specified function and making the non-const ptr
scheduling have consistent requirements with the const ptr scheduling.

include/llvm/CodeGen/GlobalISel/CombinerHelper.h
41

Consider naming the second parameter something similar to "FromRegOp" for better clarity.

lib/CodeGen/GlobalISel/CombinerHelper.cpp
38

ditto for naming FromReg to indicate it's a MachineOperand.

43

If this MachineOperand is a def, should this call replaceRegWith which also handles the constraining?

303

Should this call CombinerHelper::replaceRegOpWith which will handle the Observer notifying?

308–309

This also looks like it could go through replaceRegOpWith interface?

dsanders marked 8 inline comments as done.Dec 13 2018, 1:05 PM
dsanders added inline comments.
lib/CodeGen/GlobalISel/CombinerHelper.cpp
303

I think this can be done. I just need to re-test it

308–309

This one is a little special. We reported it changing right back at the start when we changed the opcode and changing/changed should probably be balanced. For the constraints aspect of it, I don't believe we need to do anything here as G_*LOAD and G_TRUNC/G_*EXT should have compatible result constraints by definition. If that's not true, the right thing to do would be to reject the combine.

Thanks Daniel. LGTM with changes to the nitpicks..

This revision is now accepted and ready to land.Dec 13 2018, 1:11 PM
This revision was automatically updated to reflect the committed changes.