Page MenuHomePhabricator

[SVE] Fix cast<FixedVectorType> in truncateToMinimalBitwidths
ClosedPublic

Authored by DylanFleming-arm on Jun 14 2021, 9:51 AM.

Diff Detail

Event Timeline

DylanFleming-arm requested review of this revision.Jun 14 2021, 9:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 14 2021, 9:52 AM
Matt added a subscriber: Matt.Jun 14 2021, 12:31 PM

Hi @DylanFleming-arm
I believe something wrong happened when you uploaded this patch.
It is not showing LoopVectorize.cpp file.
I can only see Context not available.
Is that happening for you too?

llvm/test/Transforms/LoopVectorize/AArch64/sve-trunc-min-bitwidth.ll
2 ↗(On Diff #351904)

Remove that

3 ↗(On Diff #351904)

Can you make that a more generic test using the flag:
-force-target-supports-scalable-vectors
?

5 ↗(On Diff #351904)

Remove that too.

106 ↗(On Diff #351904)

I believe you don't need that.

sdesmalen added inline comments.Jun 16 2021, 4:51 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
3955–3956

This looks like the same code, just different formatting.

Did you diff with the correct commit? It also says that llvm/test/Transforms/LoopVectorize/AArch64/sve-trunc-min-bitwidth.ll was moved, but this file does not exist upstream.

kmclaughlin added inline comments.
llvm/test/Transforms/LoopVectorize/scalable-trunc-min-bitwidth.ll
3

For most of the other scalable tests added to LoopVectorize we've used -scalable-vectorization=on

5

This function doesn't have any attributes, so you can remove the #0

69

I think you can change this to %for.body and then remove the for.body.pre block below.

sdesmalen added inline comments.Jun 25 2021, 9:03 AM
llvm/test/Transforms/LoopVectorize/scalable-trunc-min-bitwidth.ll
10

This loop is no longer vectorized, so this test is no longer testing your code.

Fixed issue where loop wasn't correctly vectorized with scalable vectors

Just a few more nits on the test.

llvm/test/Transforms/LoopVectorize/scalable-trunc-min-bitwidth.ll
5

%dptr is not a pointer, but rather the number of loop iterations, e.g. %N.

5

sptr is also not a pointer, but rather some store value, e.g. %val.

5

if you make i16* %hptr into i16* noalias %hptr, then you avoid the memory checks because the compiler knows the data pointed to by %hptr doesn't alias with any other memory that's accessed in the function, so it won't have to actively check if there is a dependence between the load/store by adding the compares. See the LangRef.

Updated parameter names to make more sense
Made i16* %hptr into noalias

This revision is now accepted and ready to land.Jul 5 2021, 6:29 AM