[Clang][BasicAA][AIE2] Enable full PHI AA for AIE #103
Conversation
andcarminati
requested review from
abhinay-anubola,
abnikant,
gbossu,
khallouh,
konstantinschwarz,
martien-de-jong,
SagarMaheshwari99 and
stephenneuendorffer
as code owners
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
andcarminati
force-pushed
the
andreu.extend.full.phi
branch
from
fc410ef to
299687c
Compare
andcarminati
force-pushed
the
andreu.extend.full.phi
branch
3 times, most recently
from
513b957 to
e11ba65
Compare
There was a problem hiding this comment.
I looked through llvm/lib/Analysis/BasicAliasAnalysis.cpp to understand your change within more context. This LGTM! I'm sure it was quite challenging to trace back our problem to here 💪
If possible, I'd be even happier with a simpler additional test case. Maybe a diamond cfg with one non-recursive phi node that has two incoming values, one for each side of the diamond. Would that be enough to show that --basic-aa-full-phi-analysis=false is not enough?
andcarminati
force-pushed
the
andreu.extend.full.phi
branch
2 times, most recently
from
c37a049 to
ac3124f
Compare
Hi @gbossu, I included an additional test. I think now we can clearly see the difference regarding the behavior. |
andcarminati
force-pushed
the
andreu.extend.full.phi
branch
2 times, most recently
from
6309116 to
99297cc
Compare
andcarminati
force-pushed
the
andreu.extend.full.phi
branch
3 times, most recently
from
1464cd5 to
5d1ff82
Compare
| CC1Args.append({"-mllvm", "-basic-aa-full-phi-analysis=true"}); | ||
|
|
||
| // Extend the max limit of the search depth in BasicAA | ||
| CC1Args.append({"-mllvm", "-basic-aa-max-lookup-search-depth=10"}); |
There was a problem hiding this comment.
I think we (ie in the average AIE application) wouldn't really notice it if we make it 20, (or 50?)
There was a problem hiding this comment.
By looking to the statistics I saw this limit being exhausted for one benchmark.
There was a problem hiding this comment.
So 20 would be better?
There was a problem hiding this comment.
Hi @martien-de-jong, I believe that 10 is enough for now, considering our benchmarks that are quite complex.
Also increase the max limit of the search depth in DecomposeGEPExpression. With this change we can allow LICM to hoist more instructions.
andcarminati
force-pushed
the
andreu.extend.full.phi
branch
from
5d1ff82 to
00982b8
Compare
I gave a resolution, I think we can keep as 10. |
Also increase the max limit of the search depth in DecomposeGEPExpression. With this change we can allow LICM to hoist more instructions.