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.
Paths
| Differential D103326
[GlobalISel] Add combine for PTR_ADD with regbanks ClosedPublic 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. This is the first combine in CombinerHelper that uses register banks, I
Diff Detail
Event TimelineHerald added subscribers: kerbowa, hiraditya, rovka and 2 others. · View Herald TranscriptMay 28 2021, 9:19 AM Comment Actions 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
Comment Actions +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? Comment Actions
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. Comment Actions I now added two helper function in CombinerHelper, With these, adding post-RBS support to a combine is a bit shorter. This revision is now accepted and ready to land.Jun 30 2021, 3:17 PM Comment Actions
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?
sebastian-ne marked 2 inline comments as done. Comment Actions
Good point, now using a pointer. Closed by commit rGfbae34635d83: [GlobalISel] Add combine for PTR_ADD with regbanks (authored by sebastian-ne). · Explain WhyAug 17 2021, 4:58 AM This revision was automatically updated to reflect the committed changes.
Revision Contents
Diff 366866 llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
llvm/lib/Target/AMDGPU/AMDGPUCombine.td
llvm/test/CodeGen/AMDGPU/GlobalISel/extractelement-stack-lower.ll
llvm/test/CodeGen/AMDGPU/GlobalISel/extractelement.ll
llvm/test/CodeGen/AMDGPU/GlobalISel/insertelement.large.ll
|
nit: \pre Reg.isValid()