[AIE2P] Fix align requirement for wide vector load/store and split 32 aligned load/store instead of scalarizing them #321
Conversation
khallouh
requested review from
abhinay-anubola,
abnikant,
andcarminati,
F-Stuckmann,
gbossu,
katerynamuts,
konstantinschwarz,
martien-de-jong,
niwinanto,
SagarMaheshwari99 and
stephenneuendorffer
as code owners
|
Nice, I think we need the same for AIE2. |
AIE2 doesn't have 512-bit load/store instructions, so those will be broken down into 256-bit accesses which only require 32-byte alignment. I think AIE2 is fine |
|
Nice, I think we need the same for AIE2.
The problem there is a bit different. For AIE2 we are considering alignments of 32 bits (probably a misunderstanding of how alignment is expressed during legalization.). |
khallouh
changed the title
khallouh
force-pushed
the
hamza.fix.alignment
branch
2 times, most recently
from
1f59bfd to
eae60df
Compare
|
This now splits the large loads/stores when the alignment is 256 bits. |
khallouh
force-pushed
the
hamza.fix.alignment
branch
from
eae60df to
4c82b3f
Compare
khallouh
changed the title
khallouh
force-pushed
the
hamza.fix.alignment
branch
from
4c82b3f to
d9810c1
Compare
khallouh
changed the title
| ; AIE2P: liveins: $p0 | ||
| ; AIE2P-NEXT: {{ $}} | ||
| ; AIE2P-NEXT: [[COPY:%[0-9]+]]:_(p0) = COPY $p0 | ||
| ; AIE2P-NEXT: [[LOAD:%[0-9]+]]:_(<32 x s8>) = G_LOAD [[COPY]](p0) :: (load (<32 x s8>)) |
There was a problem hiding this comment.
I assume we don't need the alignment anymore, but it seems wrong that it disappears. It's still 32 byte aligned, isn't it?
There was a problem hiding this comment.
yeah, it always disappears post legalizer, even if we just use .legal(), not sure why
| const uint64_t Alignment = Query.MMODescrs[0].AlignInBits; | ||
| const unsigned SplitFactor = MemoryType.getSizeInBits() / Alignment; | ||
| return std::make_pair( | ||
| 0, LLT::fixed_vector(MemoryType.getNumElements() / SplitFactor, |
There was a problem hiding this comment.
nit: return std::make_pair(0, MemoryType.divide(SplitFactor));
There was a problem hiding this comment.
Note that, if some unaligned scalar load appears (let's say s512), divide will give us a valid scalar type, not replacing the type by a vector.
There was a problem hiding this comment.
Good point but I don't think fewerElements works with scalars. I can assert that we have a vector here. Otherwise, we will simply scalarize as we already do and currently, it will fail during the process because we can't legalize the resulting merge and unmerge opcodes.
| [=](const LegalityQuery &Query) { | ||
| const LLT &MemoryType = Query.MMODescrs[0].MemoryTy; | ||
| const uint64_t Alignment = Query.MMODescrs[0].AlignInBits; | ||
| if (MemoryType.getSizeInBits() >= 512 && Alignment == 256) |
There was a problem hiding this comment.
nit: return (MemoryType.getSizeInBits() >= 512 && Alignment == 256);
There was a problem hiding this comment.
Should we also consider MemoryType.getSizeInBits() % 256 == 0, or at least assert?
There was a problem hiding this comment.
We can't assert because we have other types above and it will break for them but we can return false instead
khallouh
force-pushed
the
hamza.fix.alignment
branch
from
d9810c1 to
a4bb6fe
Compare
No description provided.