[AIE2] Optimize 2D/3D memory operations #145
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
andcarminati
commented
Aug 7, 2024
| const TargetInstrInfo &TII = *MBB.getParent()->getSubtarget().getInstrInfo(); | ||
| bool Changed = false; | ||
|
|
||
| // TODO: LoopUtils? |
There was a problem hiding this comment.
We can postpone this refactoring for now to avoid conflicts.
There was a problem hiding this comment.
We now have llvm/lib/Target/AIE/Utils/AIELoopUtils.h. Maybe we should move the helper there?
There was a problem hiding this comment.
I moved this lambda as a function to AIELoopUtils.
andcarminati
force-pushed
the
andreu.dup.2d3d.address
branch
from
6063b35 to
56e87c3
Compare
|
Some QoR results: With With unrolling enabled: |
gbossu
reviewed
Aug 19, 2024
gbossu
reviewed
Aug 19, 2024
gbossu
reviewed
Aug 19, 2024
gbossu
reviewed
Aug 19, 2024
gbossu
reviewed
Aug 19, 2024
gbossu
reviewed
Aug 19, 2024
| for (auto &RegMILoad : LoadUses) { | ||
| Register Reg = RegMILoad.first; | ||
| if (!UsedByStore.contains(Reg)) | ||
| continue; |
There was a problem hiding this comment.
Nit: I think checking for uses by both load and stores is a good enough heuristic, but I'm wondering if we could not be a bit more precise and detect the case where the iterators will be duplicated inside the loop.
I guess we have something like
loop:
%0:edc = PHI [entry, ...], [loop, %4]
%10:edc = PHI [entry, ...], [loop, %14]
%1:ed = reg_seq %100, %101, %102, %0
%2:vec256, %3:ep, %4:edc = VLD_2D %200:ep %1:ed
%11:ed = reg_seq %100, %101, %102, %10
%13:ep, %14:edc = VST_2D %200:ep %11:ed, ...
We could notice that the %100, %101, %102 components are used in iterators whose counter comes from different PHI nodes. I think this means the regcoalescer/twoaddresselimination will fail and will need to insert 3 copies to materializxe the %11 reg_sequence.
Food for thought.
There was a problem hiding this comment.
Hi @gbossu, this case is already covered and this pseudo code looks like the one that we have in the test ;-). We filter off for duplication registers that are defined inside the MBB. Maybe with the refactoring this cases will be a bit more clear.
andcarminati
force-pushed
the
andreu.dup.2d3d.address
branch
from
56e87c3 to
b9722fa
Compare
gbossu
reviewed
Aug 21, 2024
gbossu
reviewed
Aug 22, 2024
gbossu
previously approved these changes
Aug 22, 2024
There was a problem hiding this comment.
LGTM! As mentioned in #145 (comment), I think checking for uses of iterators by both load and stores is a simple and good enough heuristic. We can always improve it if needed in the future.
I left some nits, address up to taste
andcarminati
force-pushed
the
andreu.dup.2d3d.address
branch
from
b9722fa to
8a1f34e
Compare
Thank you very much for the suggestions. I addressed all of them. |
gbossu
previously approved these changes
Aug 23, 2024
andcarminati
force-pushed
the
andreu.dup.2d3d.address
branch
from
8a1f34e to
1d783a1
Compare
gbossu
previously approved these changes
Aug 23, 2024
andcarminati
force-pushed
the
andreu.dup.2d3d.address
branch
from
1d783a1 to
83a4e79
Compare
andcarminati
force-pushed
the
andreu.dup.2d3d.address
branch
from
83a4e79 to
e37f896
Compare
martien-de-jong
previously approved these changes
Aug 29, 2024
|
|
||
| bool Changed = false; | ||
| for (auto &RegMILoad : LoadUses) { | ||
| Register Reg = RegMILoad.first; |
|
|
||
| // Used by both loads and stores. We will duplicate for | ||
| // load use. | ||
| MachineInstr *DefReg = MRI.getUniqueVRegDef(Reg); |
There was a problem hiding this comment.
This const is not possible because we need a non-const iterator.
| if (UseMI->mayLoad()) | ||
| LoadUses[Reg].insert(&MI); | ||
| else | ||
| UsedByStore.insert(Reg); |
There was a problem hiding this comment.
Theoretically, the use could be in the value operand of the store, or not a store at all. If this is still the right action, the name of the set might be more generically NonLoadUses.
I can also imagine that we collect MI and NonLoadUses in one map indexed by Reg.
There was a problem hiding this comment.
I renamed the set to NonLoadUses, because if can hold some padd cases, if we fail to combine.
gbossu
previously approved these changes
Aug 30, 2024
andcarminati
dismissed stale reviews from gbossu and martien-de-jong
via
93e9aeb
andcarminati
force-pushed
the
andreu.dup.2d3d.address
branch
from
e37f896 to
93e9aeb
Compare
In AIEBaseRegisterInfo
This for loop blocks with shared virtual addressing regs between loads and stores.
andcarminati
force-pushed
the
andreu.dup.2d3d.address
branch
from
93e9aeb to
a54bbe8
Compare
gbossu
approved these changes
Sep 2, 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.
For loop blocks with a small number of memory operations to avoid spills. This PR clear a bit inner loops to help PostRA scheduling optimizations.
QoR information:
fno-unroll-loopsis used, we can observe changes like (Reduce*):To: