Page MenuHomePhabricator

[GlobalISel] Add combine for PTR_ADD with regbanks
AcceptedPublic

Authored by sebastian-ne on May 28 2021, 9:19 AM.

Details

Summary

Combine two G_PTR_ADDs, but keep the register bank of the constant.
That way, the combine can be used in post-regbank-select combines.

This is the first combine in CombinerHelper that uses register banks, I
hope it is the right place.

Diff Detail

Event Timeline

sebastian-ne created this revision.May 28 2021, 9:19 AM
sebastian-ne requested review of this revision.May 28 2021, 9:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 28 2021, 9:19 AM
arsenm added a comment.Jun 1 2021, 4:19 PM

I wonder if we should try to intelligently pick the register banks for constants before adding combine logic relying on the current scheme where all constants end up in SGPRs.

I'm not sure how we want to address this though. Theoretically RegBankSelect itself should be selecting the G_CONSTANT bank based on the users, it just doesn't try to do this now. We could also have a different post regbank combine. We do have situations where depending on the value and how many uses there are, we may just want to rematerialize the constant multiple times. I'm not sure if that's something RegBankSelect should be doing itself

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1776

Use regular getVRegDef

1787–1788

Probably should cache these in CombinerHelper if they're not there already

1804

Should not need to construct a new builder

1809

The hard part of this is how to avoid spreading the ugly manual regbank preservation everywhere.

sebastian-ne marked 3 inline comments as done.

Fix most inline review comments

+more GlobalISel reviewers.

What do you all think about this? It seems to be the first generic combine designed specifically to run post-regbankselect. Should we be going in this direction? Or modifying existing combines so they can work both pre- and post-regbankselect?

Assuming we do want this combine, is there a neater way of writing the "ugly manual regbank preservation" that Matt highlighted?

+more GlobalISel reviewers.

What do you all think about this? It seems to be the first generic combine designed specifically to run post-regbankselect. Should we be going in this direction? Or modifying existing combines so they can work both pre- and post-regbankselect?

Assuming we do want this combine, is there a neater way of writing the "ugly manual regbank preservation" that Matt highlighted?

Modifying existing combines to work post-RBS, or adding RBS preserving variants, doesn't seem ideal to me but I don't have a better suggestion. It would be best if we could finish the tablegen combiner implementation one day and be able to specify this stuff declaratively if possible.

As for this solution, we use isLegalOrBeforeLegalizer() hooks in other combines, maybe we could add something similar for isBeforeRegBankSelect() and add the regbank checks guarded by that condition in the original combine.

I now added two helper function in CombinerHelper,
getRegBankId returns an Optional<unsigned> for a register and
setRegBankId sets such an optional bank to a register (or does nothing it it’s None).

With these, adding post-RBS support to a combine is a bit shorter.

aemerson accepted this revision.Wed, Jun 30, 3:17 PM
aemerson added inline comments.
llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h
132

nit: \pre Reg.isValid()

This revision is now accepted and ready to land.Wed, Jun 30, 3:17 PM
foad added a comment.Thu, Jul 1, 2:21 AM

I now added two helper function in CombinerHelper,
getRegBankId returns an Optional<unsigned> for a register and
setRegBankId sets such an optional bank to a register (or does nothing it it’s None).

Why use an optional ID, instead of just a RegisterBank pointer that may be null? Is it just to make it more obvious that setRegBankId(None) should do nothing?

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
148

Doesn't matter of course, but you could use ?: here.

sebastian-ne marked 2 inline comments as done.

Why use an optional ID, instead of just a RegisterBank pointer that may be null?

Good point, now using a pointer.

foad accepted this revision.Thu, Jul 8, 3:15 AM