[AIEX] Enhance combiner to support post-inc-load requiring moving multiple instructions #369
Conversation
krishnamtibrewala
requested review from
abhinay-anubola,
abnikant,
andcarminati,
F-Stuckmann,
gbossu,
katerynamuts,
khallouh,
konstantinschwarz,
martien-de-jong,
niwinanto,
SagarMaheshwari99 and
stephenneuendorffer
as code owners
| if (!canDelayMemOp(*DependentInstrs[i + 1], *DependentInstrs[i], MRI)) | ||
| return false; | ||
|
|
||
| return true; |
There was a problem hiding this comment.
What I don't like here is that we have a very layered view of dependences. I think the basic condition is: the destination should not be reachable from the source in the dependence graph over any of DependentInstrs. canDelayMemOp() is one way of coding a dependence edge in that graph, and the use def chains are another one.
Also, the formulation is not the most direct. I think you actually want to know canAdvanceDest(MI, DestMI) for all MI in DependentInstrs
There was a problem hiding this comment.
Src -> I1 -> I2 -> I3
Src -> Dst
There was a problem hiding this comment.
What I have tried to check here using the existing functions is if I can pack these dependence graphs together.
By together here, I mean next to each other.
Using the same Src -> I1 -> I2 -> I3 andSrc -> Dstexample but with others Ix in between Src/Des/I1-3, I check if it's safe to pack them as Src , I1, I2, I3 , Dst in mist, of Ix if so, I move belowI1, I2, I3athe Dst and Src just above Dst
The reason for taking this approach was that I1 -> I2 -> I3 could only be moved as a group, and checking that in a single shot without actually moving them in the "match" phase of the combiner would have been tricky.
krishnamtibrewala
force-pushed
the
aie-post-inc
branch
2 times, most recently
from
474c3d8 to
e39efb2
Compare
|
|
||
| // Check if we can move the dependent instructions after Dest in order to | ||
| // enable post-increment combining. | ||
| if (!canDelayMemOp(*DependentInstrs.front(), Dest, MRI)) |
There was a problem hiding this comment.
I think this check is the only that matters here. Maybe you can add a test that fails on the next for.
There was a problem hiding this comment.
Only this wont be sufficient (kingly note the front() will give the last instruction in the dependency chain)
Why we need the next check is explained here #369 (comment)
krishnamtibrewala
force-pushed
the
aie-post-inc
branch
from
e39efb2 to
53ba2c3
Compare
krishnamtibrewala
force-pushed
the
aie-post-inc
branch
from
53ba2c3 to
0358f8c
Compare
krishnamtibrewala
force-pushed
the
aie-post-inc
branch
from
0358f8c to
7a5f416
Compare
|
Hi @andcarminati can you re-review this again. Thank you. |
llvm/test/CodeGen/AIE/aie2p/GlobalIsel/combine-postinc-load.mir
Outdated
Show resolved
Hide resolved
| std::distance(SrcMIIter, DestMIIter)) | ||
| break; | ||
| DependentInstrs.insert(&Use); | ||
| DependentInstrInRange(Use, DestMI, MRI, DependentInstrs); |
There was a problem hiding this comment.
Is there a chance for this to enter in loop? What happens if Use is a Phi node? We could have a test, something like:
a = Phi (c) ...
b = load p0
c = add a, b
p1 = padd p0
We could have a test with such a scenario.
There was a problem hiding this comment.
Have added a test and explain why we will not get into a loop.
…tiple instructions
krishnamtibrewala
force-pushed
the
aie-post-inc
branch
from
7a5f416 to
c338200
Compare
Enhance the combiner to identify the potential to move multiple dependent instructions below G_PTR_ADD to enable post-inc combining.
Say between a dependent LOAD & PTR_ADD you have multiple instructions that use the value loaded by the LOAD instruction.
First we try to identify the dependent instructions chain, which uses the definition of LOAD, and see if we can move the dependency chain below PTR_ADD such that we open up the potential to combine LOAD & PTR_ADD to POST_INC_LOAD.