[AIEX] Add AA support for stack fixed objects #672
Conversation
andcarminati
requested review from
abhinay-anubola,
abnikant,
F-Stuckmann,
katerynamuts,
khallouh,
konstantinschwarz,
martien-de-jong,
niwinanto,
SagarMaheshwari99 and
stephenneuendorffer
as code owners
| OffsetA = MFI.getObjectOffset(FixedStackA->getFrameIndex()); | ||
| OffsetB = MFI.getObjectOffset(FixedStackB->getFrameIndex()); | ||
| if (OffsetA != OffsetB) { | ||
| SameVal = true; |
There was a problem hiding this comment.
CHECK: This is a conservative estimate that if the Frameindices are unequal and the offsets are equal they could overlap and to be safe we assign them the same Value?
There was a problem hiding this comment.
In this case, if FIs are different and also Offsets are different we use SameVal to check overlapping. We assign SameVal because they are in fact related to the same origin: stack.
| const PseudoSourceValue *PSVa = MMOa->getPseudoValue(); | ||
| const PseudoSourceValue *PSVb = MMOb->getPseudoValue(); | ||
| if (PSVa && PSVb) { | ||
| if (PSVa == PSVb) { |
There was a problem hiding this comment.
We have a lot of nesting levels here. Could we reduce them and add a couple of early exits?
There was a problem hiding this comment.
I rewrote the code accordingly.
andcarminati
force-pushed
the
andreu.fixed.stack.aa
branch
2 times, most recently
from
6196188 to
9ca79fc
Compare
| const PseudoSourceValue *PSVa = MMOa->getPseudoValue(); | ||
| const PseudoSourceValue *PSVb = MMOb->getPseudoValue(); | ||
|
|
||
| const bool WithBothPseudoSources = PSVa && PSVb; |
There was a problem hiding this comment.
nit: ExistBothPseudoSources?
|
|
||
| const bool SamePseudoSource = PSVa == PSVb; | ||
| if (SamePseudoSource) | ||
| return CheckOverlapping(MMOOffsetB, MMOOffsetB); |
There was a problem hiding this comment.
There is a bug: return CheckOverlapping(MMOOffsetA, MMOOffsetB);
There was a problem hiding this comment.
True, happily it is not is not optimistic.
|
|
||
| bool AIEBaseInstrInfo::areMemAccessesTriviallyDisjoint( | ||
| const MachineInstr &MIa, const MachineInstr &MIb) const { | ||
| if (!MIa.hasOneMemOperand() || !MIb.hasOneMemOperand()) |
There was a problem hiding this comment.
I think we could make the first exit check more strict
if (MIa.hasUnmodeledSideEffects() || MIb.hasUnmodeledSideEffects() || MIa.hasOrderedMemoryRef() || MIb.hasOrderedMemoryRef()) return false;
There was a problem hiding this comment.
areMemAccessesTriviallyDisjoint is related to the memory addresses, memory ordering is handled apart of AA.
| if (!WithBothPseudoSources) | ||
| return false; | ||
|
|
||
| const bool SamePseudoSource = PSVa == PSVb; |
There was a problem hiding this comment.
Should we also be checking PSVa->isAliased()
There was a problem hiding this comment.
According to the sources:
// If true, an LLVM IR value might point to this object.
// Normally, spill slots and fixed-offset objects don't alias IR-accessible
// objects, but there are exceptions (on PowerPC, for example, some byval
// arguments have ABI-prescribed offsets).
Apart of that, I see that this is not relevant to us because we are not comparing against IR objects.
|
Overall I like the simpler, logical approach. |
andcarminati
force-pushed
the
andreu.fixed.stack.aa
branch
from
9ca79fc to
e3cfd9d
Compare
This helps the scheduler to generate better code for cases where some parameters are passed by value through the stack.
This has a nice synergy with #669