[AIEX] Propagate MMO for loads without PTR #198
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
Sep 27, 2024
| if (MO.isReg() && MO.getReg().isVirtual()) { | ||
| const Register Reg = MO.getReg(); | ||
| if (const MachineInstr *RegDef = MRI.getVRegDef(Reg)) | ||
| findGlobalValues(*RegDef, GVSet, VisitedInstrs, 10, MRI); |
There was a problem hiding this comment.
Perhaps put the 10 in a command line option right away?
There was a problem hiding this comment.
Recursion depth is an explosive parameter. The operands you are traversing may branch quite heavily.
There was a problem hiding this comment.
With a command line we can control this now!
| } | ||
| } | ||
|
|
||
| // Add gattered GlobalValues as MMOs. |
| } | ||
|
|
||
| // Add gattered GlobalValues as MMOs. | ||
| for (auto GV : GVSet) { |
There was a problem hiding this comment.
I think we should know whether we completed the search, otherwise we may be adding an incomplete set, which can cause over-optimistic alias analysis
| @@ -472,6 +539,12 @@ bool AIEPostSelectOptimize::runOnMachineFunction(MachineFunction &MF) { | |||
| } | |||
| } | |||
|
|
|||
| // 4. Fix MMO for load instructions that don't use pointers | |||
| // registers (use vector instead, for example). | |||
There was a problem hiding this comment.
nit: I think it adds MMOs for loads that don't already have them, irrespective of whether they are pointers or not.
There was a problem hiding this comment.
Changed the comment accordingly.
| for (auto MO : MI.uses()) { | ||
| if (MO.isGlobal()) { | ||
| GVSet.insert(MO.getGlobal()); | ||
| } else if (MO.isReg() && MO.getReg().isVirtual()) { |
There was a problem hiding this comment.
I think you should recognize the regular address modifiers here and stop recursion. You don't want to descend into integer operands of e.g. GEP.
There was a problem hiding this comment.
Hi @martien-de-jong, as we are in MIR now, all GEPs were translated to ptradd, mul and etc.
There was a problem hiding this comment.
GEP, PTRADD, not much different. PTR_ADD only leads to pointers in its first operand, not in the second, so searching the second is wasted.
There was a problem hiding this comment.
But what if the first operand is null (or something like G_INTTOPTR(G_CONSTANT)) and second is an integer derived from a G_PTRTOINT (or something like G_PTRTOINT(GlobalValue))? Pointer arithmetic can be crazy... like:
body: |
bb.1.entry:
%1:_(p0) = G_CONSTANT i20 4
%4:_(p0) = G_GLOBAL_VALUE @Global
%3:_(s20) = G_PTRTOINT %4(p0)
%7:_(s20) = G_CONSTANT i20 2
%5:_(s20) = G_SHL %3, %7(s20)
%6:_(p0) = G_PTR_ADD %1, %5(s20)
$p0 = COPY %6(p0)
PseudoRET implicit $lr, implicit $p0
...
In this example we can reach the pointer though the second operand.
There was a problem hiding this comment.
I wouldn't mind if those programmers were punished...
But seriously, if we descend through any operator, the recursion depth is not nicely modelling processing effort. I would rather have somethin linear like number of visited nodes. And you have that already available as the size of the visited set.
There was a problem hiding this comment.
As discussed offline, the recursion limit was replaced by the number of visited instructions.
|
I don't think it's important, but I guess that llvm will also consider the constant table addresses as leaked (!nocapture-like) |
andcarminati
force-pushed
the
andreu.fix.memop
branch
from
0204d3c to
f88c960
Compare
Humm, I am trying to understand how this can affect us.... |
|
QoR results: |
gbossu
reviewed
Oct 2, 2024
andcarminati
force-pushed
the
andreu.fix.memop
branch
from
f88c960 to
3544996
Compare
|
Just for future: We've seen cases where the Globals themselves don't have an address space, but they are casted to a pointer of the right AS. I guess the first AS we encounter coming down from the load is the one intended by the programmer. I also think we can trust standard AA to follow that pointer cast down to the Global, so we could also use it as address for the MMO. |
andcarminati
force-pushed
the
andreu.fix.memop
branch
4 times, most recently
from
4e7ce06 to
3462674
Compare
gbossu
reviewed
Oct 21, 2024
andcarminati
force-pushed
the
andreu.fix.memop
branch
from
3462674 to
093ac91
Compare
gbossu
reviewed
Oct 21, 2024
| static_cast<const AIEBaseInstrInfo *>(ST.getInstrInfo()); | ||
|
|
||
| // Skip subvector copy. | ||
| if (MI->isCopy()) { |
There was a problem hiding this comment.
Nit: I guess there could be multiple copies. Isn't there a helper function somewhere to look through copies?
| if (LoadMMODeepSearch) | ||
| // Look to everything! | ||
| Success &= findGlobalValues(RegDef, GVSet, VisitedInstrs, MRI, | ||
| LoadMMOSearchLimit); |
There was a problem hiding this comment.
I think we should avoid committing the findGlobalValues as it is overly optimistic. Maybe keep it on a separate branch for now?
There was a problem hiding this comment.
I can't say that this is optimistic or not, I can say that this is harder to break if we consider the pattern recognition. It tracks everything (in terms of globals) on the way of VLDB.4X load, it only rejects when it reaches function parameters (because they can be pointers as well). For AA perspective, if we miss one MMO is problem. On the other hand, if you include non-used ones, there is no problem because it will lead to more dependencies (just performance impacts).
There was a problem hiding this comment.
My main "issue" with it is that it considers loads safe, while I think they are only safe when we have already found a "vector" of pointers. In that case, it does not matter what we add to the vector, it could be a constant vector, or some loaded quantity.
There was a problem hiding this comment.
The new version addresses this situation: we need to find a select(bcast(C1), bcast(C1), mask) somewhere in the chain up starting form the load.
andcarminati
force-pushed
the
andreu.fix.memop
branch
2 times, most recently
from
08af3de to
d775df6
Compare
gbossu
reviewed
Oct 22, 2024
| MachineRegisterInfo &MRI) { | ||
|
|
||
| if (MI->isCopy()) | ||
| MI = getDefIgnoringCopiesAndBitcasts(MI->getOperand(1).getReg(), MRI); |
There was a problem hiding this comment.
Nit: Why not unconditionally call MI = getDefIgnoringCopiesAndBitcasts(MI->getOperand(1).getReg(), MRI);?
gbossu
reviewed
Oct 22, 2024
andcarminati
force-pushed
the
andreu.fix.memop
branch
from
d775df6 to
5cc8dc9
Compare
|
Hi @gbossu, now I hope we converged ;-) It is also possible to SWP (pre) Softmax with: |
|
|
||
| for (MachineInstr &MI : MBB) { | ||
| // We want to analyze just loads without post increment. | ||
| if (!MI.mayLoad() || !MI.memoperands_empty() || MI.getNumOperands() != 2) |
There was a problem hiding this comment.
This is an attempt to recognise a multi-way load in an architecture-neutral way. Why not add that to the set of abstract operations?
| return false; | ||
| } | ||
| return UnknownSide /*offset side is found*/ && | ||
| KnownSide /*broadcasted side is found*/; |
There was a problem hiding this comment.
CHECK: KnownSide tells us that operand can be traced wholly to a set of objects. UnknownSide might be anything, but cannot introduce new objects without also introducing undefined behaviour. I have a feeling we only need to check KnownSide. Moreover, I would not use the term 'broadcasted side'. We may later generalize to other ways to construct a vector of object designations. 'object side' rhymes better with 'offset side'.
There was a problem hiding this comment.
Note: you couldn't directly assign to KnownSide anymore, instead do a sticky update if the visit function returns true.
| KnownSide = visitVAdd(*VOp, TII, MRI, GVSet); | ||
| else if (VOp->Type == Operation::SELECT) | ||
| KnownSide = visitVSelect(*VOp, TII, MRI, GVSet); | ||
| else |
There was a problem hiding this comment.
I guess we could also deal with BROADCAST here?
There was a problem hiding this comment.
By correctness yes. But, due to architectural restrictions (odd-even memory banks), I believe that this cannot represent a real case. A final pointer must be constructed with at least two broadcasts.
There was a problem hiding this comment.
Select (add(broadcast, offset), add(broadcast,offset)) would fit?
There was a problem hiding this comment.
It can be, however I prefer to only include already existing cases here. As discussed before, we can include any new pattern on demand.
| // from SELECT or BROADCAST. | ||
| if (!VOp) | ||
| return false; | ||
| if (VOp->Type == Operation::SELECT) |
There was a problem hiding this comment.
I guess visitXXX could return false if VOp isn't XXX
There was a problem hiding this comment.
As this could represent a implementation mistake, I inserted asserts.
| if (!MI.mayLoad() || !MI.memoperands_empty() || MI.getNumOperands() != 2) | ||
| continue; | ||
| SmallPtrSet<const Value *, 4> GVSet; | ||
| if (traverseVecPointerChain(MI.getOperand(1).getReg(), GVSet, TII, MRI)) { |
There was a problem hiding this comment.
nit: Operand(1) may not be a register
There was a problem hiding this comment.
As we parse the abstract load now, we started to have this guarantee.
| @@ -441,6 +441,20 @@ struct AIEBaseInstrInfo : public TargetInstrInfo { | |||
| llvm_unreachable("Target didn't implement isProfitableToSplitType!"); | |||
| } | |||
|
|
|||
| /// Abstract vector operation to help the decoding of complex operations. | |||
| struct AbstractVecOp { | |||
There was a problem hiding this comment.
nit: We could also use this for non-vector ops
There was a problem hiding this comment.
Changed accordingly.
| if (MO.isGlobal()) { | ||
| GVSet.insert(MO.getGlobal()); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
MI could be anything, e.g. the top of a tree of selects. I guess we have to return false whenever MO isn't a global and collect all used globals.
There was a problem hiding this comment.
Yes I believe any operand that cannot be tracked as a memory location should make us return false
There was a problem hiding this comment.
Done! Now we consider only the case with one src operand that loads a global.
|
New QoR results (updated): |
andcarminati
force-pushed
the
andreu.fix.memop
branch
2 times, most recently
from
2e69d96 to
e176158
Compare
gbossu
reviewed
Oct 25, 2024
gbossu
reviewed
Oct 25, 2024
|
|
||
| // Recognize ADD ((ADD | SELECT), Unknown) | ||
| bool visitVAdd(const AbstractOp &VAdd, const AIEBaseInstrInfo *TII, | ||
| MachineRegisterInfo &MRI, SmallPtrSet<const Value *, 4> &GVSet) { |
There was a problem hiding this comment.
Would that function be equivalent to something like:
bool visitAddressVector(const AbstractOp &Op, const AIEBaseInstrInfo *TII,
MachineRegisterInfo &MRI, SmallPtrSet<const Value *, 4> &GVSet) {
if (Op.Type == Operation::VECTOR_SELECT)
return visitSelect(...);
if (Op.Type != Operation::VECTOR_ADD)
return false;
return any_of(Op.VectorSrcRegs, [&](Register SrcReg) {
OptionalOp VOp = TII->parseAbstractOp(*MRI.getVRegDef(Reg));
return !VOp || visitAddressVector(*VOp, TII, MRI, GVSet);
};
}
91ef063 to
460287b
Compare
gbossu
reviewed
Oct 28, 2024
|
|
||
| return any_of(VOp.VectorSrcRegs, [&](Register SrcReg) { | ||
| OptionalOp VOp = TII->parseAbstractOp(*MRI.getVRegDef(SrcReg)); | ||
| return !VOp || visitAddressVector(*VOp, TII, MRI, GVSet); |
There was a problem hiding this comment.
I think I made a mistake when suggesting that cod. That should probably be VOp && visitAddressVector(*VOp, TII, MRI, GVSet). I.e. we are fine as long as one of the source registers could be successfully traced to an address vector. What do you think?
There was a problem hiding this comment.
If one can be traced back, we are fine! I will update.
andcarminati
force-pushed
the
andreu.fix.memop
branch
from
460287b to
aab6fdc
Compare
This can prevent MachinePipeliner from considering some loads as barriers (isDependenceBarrier).
andcarminati
force-pushed
the
andreu.fix.memop
branch
from
aab6fdc to
b7e280d
Compare
gbossu
approved these changes
Oct 28, 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.
This can prevent MachinePipeliner from considering some loads as barriers (isDependenceBarrier).