Page MenuHomePhabricator

[GlobalISel] Simplify RegBankSelect
ClosedPublic

Authored by sebastian-ne on Oct 6 2021, 5:58 AM.

Details

Summary

Save the instruction list of a block before selecting banks.
This allows to cope with moved instructions, even if they are reordered
or splitted into multiple basic blocks.

Diff Detail

Event Timeline

sebastian-ne created this revision.Oct 6 2021, 5:58 AM
sebastian-ne requested review of this revision.Oct 6 2021, 5:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 6 2021, 5:58 AM
foad added inline comments.Oct 6 2021, 6:16 AM
llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp
46

Don't need this since it's already done in the header.

730–731

Can you initialise the worklist from RPOT and then just have a single worklist loop, instead of two nested loops?

llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp
774–776 ↗(On Diff #377514)

Before this patch, how did these new BBs get regbankselected?

sebastian-ne marked 2 inline comments as done.

Fix review comments, use just the worklist instead of a nested loop.

llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp
774–776 ↗(On Diff #377514)

RegBankSelect had logic to detect if applyMapping moved an instruction to a new basic block and then continued from there.

The banks for instructions in RemainderBB and RestoreExecBB are set here, so they don’t need to be selected.

foad added a comment.Oct 6 2021, 7:40 AM

OK, so if applyMapping creates new instructions it either has to

  1. set banks for the new instructions' operands as they are created, or
  2. do something to make sure RegBankSelect visits the new instructions.
foad added a comment.Oct 6 2021, 7:48 AM

The low level implementation details look OK to me.

I guess we need to get some agreement about the high level design: is this a good way of handling newly created instructions and BBs? In particular I wonder if there is some way of handling newly created instructions automatically, without having to call setNextInstruction, but I can't quite see how it would work.

llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp
698

Could just Worklist.insert(Worklist.end(), ...)?

sebastian-ne marked an inline comment as done.

Thanks, the Worklist.insert does read better than std::copy.

I guess we need to get some agreement about the high level design: is this a good way of handling newly created instructions and BBs? In particular I wonder if there is some way of handling newly created instructions automatically, without having to call setNextInstruction, but I can't quite see how it would work.

Yes, it gets quite complicated when introducing loops for call instructions because it moves multiple instructions and now also unbundles them (so the I++ used in RegBankSelect points to the wrong instruction – after the bundle).

The low level implementation details look OK to me.

I guess we need to get some agreement about the high level design: is this a good way of handling newly created instructions and BBs? In particular I wonder if there is some way of handling newly created instructions automatically, without having to call setNextInstruction, but I can't quite see how it would work.

Could an observer be installed to watch for newly created instructions?

I implemented it using an observer and it worked. Then I realized that I never registered the observer anywhere…
So, here’s a simpler implementation that seems to be enough to find all instructions.

The new patch for creating loops around call instructions does not use bundles anymore because they create more problems than they solve (e.g. the legalizer tries and fails to legalize instructions in a bundle). This simplifies the cases that need to be handled.

sebastian-ne retitled this revision from [GlobalISel] Pass RegBankSelect to applyMapping to [GlobalISel] Simplify RegBankSelect.Oct 25 2021, 6:06 AM
sebastian-ne edited the summary of this revision. (Show Details)
foad added inline comments.Oct 25 2021, 6:11 AM
llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp
702–706

Pass (MBB->instr_rbegin(), MBB->instr_rend()) into the constructor here?

708

I think there's a pop_back_value you can use.

sebastian-ne added inline comments.Oct 25 2021, 6:23 AM
llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp
702–706

I just tried and couldn’t get it working. I guess the iterator returns a MachineInstr& and no pointer, but SmallVector<MachineInstr&> doesn’t work.

sebastian-ne marked an inline comment as done.

Use pop_back_val

foad added inline comments.Oct 25 2021, 7:00 AM
llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp
702–706

Ugh. This might work:

SmallVector<MachineInstr *> WorkList(make_pointer_range(reverse(MBB->instrs())));
sebastian-ne marked an inline comment as done.

The make_pointer_range worked, thanks

Is the latest diff incomplete or is this really this simple now?

It’s complete. It worked in all cases I tested, so I hope it really can be that simple :)

arsenm accepted this revision.Oct 27 2021, 12:32 PM
This revision is now accepted and ready to land.Oct 27 2021, 12:32 PM
This revision was automatically updated to reflect the committed changes.