[AIE2] Add more memory/ptr combiners #214
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andcarminati
requested review from
abhinay-anubola,
abnikant,
gbossu,
khallouh,
konstantinschwarz,
martien-de-jong,
SagarMaheshwari99 and
stephenneuendorffer
as code owners
gbossu
reviewed
Oct 16, 2024
|
|
||
| unsigned AIE2InstrInfo::getMaxLoadStoreSize() const { return 256; } | ||
|
|
||
| bool AIE2InstrInfo::canCombineWithLoadStore(const MachineInstr &MI) const { |
There was a problem hiding this comment.
Having some look-ahead in early combiners seems like a good idea indeed!
gbossu
reviewed
Oct 16, 2024
gbossu
reviewed
Oct 16, 2024
| Register ValReg = MI.getReg(0); | ||
| const LLT ValTy = MRI.getType(ValReg); | ||
| const bool IsLoad = isa<GLoad>(MI); | ||
| MaxMemSize = TII.getMaxLoadStoreSize(); |
There was a problem hiding this comment.
Is there a specific reason we are passing & setting the MaxMemSize in match & using that in apply ? We could have used TII.getMaxLoadStoreSize(); directly in apply ?
There was a problem hiding this comment.
It could be. What I had in mind was a target specific match and a completely target independent apply. It gives the freedom to apply different splitting tragedies for future targets, considering different selection combines opportunities.
| const AIEBaseInstrInfo &TII, | ||
| unsigned &MaxMemSize) { | ||
|
|
||
| Register ValReg = MI.getReg(0); |
There was a problem hiding this comment.
nit : const Register
|
QoR results. In this point, we are trying to squeeze performance, but as there are no free lunch, some regressions are expected, unfortunately. For |
andcarminati
force-pushed
the
andreu.add.memcombiners
branch
from
32080b5 to
160c028
Compare
andcarminati
commented
Oct 21, 2024
| continue; | ||
|
|
||
| // We can share a residual G_PTR_ADD. | ||
| if (Use.getOperand(2).getReg() == PaddSrcOffset) { |
There was a problem hiding this comment.
This is wrong, we should compare against OffsetReg....
andcarminati
force-pushed
the
andreu.add.memcombiners
branch
from
160c028 to
e222e1e
Compare
|
New QoR numbers: |
gbossu
reviewed
Oct 22, 2024
| const LLT V64S8 = LLT::fixed_vector(64, 8); | ||
|
|
||
| if (Ty == V16S32 || Ty == V32S16 || Ty == V64S8) | ||
| return true; |
There was a problem hiding this comment.
Nit: Could this just be return Ty.isVector() && Ty.getSize() == 512;?
There was a problem hiding this comment.
The reason is that it is not a good idea to break accumulators, because we will not combine them. My first trial was in this direction and I saw regressions.
gbossu
reviewed
Oct 22, 2024
gbossu
reviewed
Oct 22, 2024
| } | ||
|
|
||
| void llvm::applyLoadStoreSplit(GLoadStore &MI, MachineRegisterInfo &MRI, | ||
| MachineIRBuilder &B, unsigned &MaxMemSize) { |
There was a problem hiding this comment.
Why is MaxMemSize passed by (non-const) reference? Can't this be a copy?
There was a problem hiding this comment.
It can, I changed it!
gbossu
reviewed
Oct 22, 2024
gbossu
reviewed
Oct 22, 2024
gbossu
reviewed
Oct 22, 2024
|
This looks great! My only potential "concern" is about the non-const references used in combiners |
*Now, we can selectively split memory operations to enhance selection combiner opportunities.
andcarminati
force-pushed
the
andreu.add.memcombiners
branch
from
e222e1e to
13a71e3
Compare
gbossu
approved these changes
Oct 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
*Now, we can selectively split memory operations to enhance selection combiner opportunities.
*QoR numbers under preparation.