Page MenuHomePhabricator

[LoopVectorize][SVE] Remove assert for scalable vector in InnerLoopVectorizer::fixReduction

Authored by CarolineConcatto on Apr 25 2021, 10:29 AM.



The function fixReduction used to assert/crash for scalable vector when
a vector reduce could be done with a smaller vector.
This patch removes this assertion as it is safe to use scalable vector for
vector reduce and truncate.

Diff Detail

Event Timeline

CarolineConcatto requested review of this revision.Apr 25 2021, 10:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 25 2021, 10:29 AM

The fix looks good thanks! Just a minor comment about the test ...

12 ↗(On Diff #340369)

Hi @CarolineConcatto, I presume this is a truncation of the PHI value? If possible I think it might be nice to have CHECK lines for the PHI values to see them being truncated. I think that ZEXT1 and ZEXT2 will also be the incoming values for the PHI node too.

  • Address review's comment about the check in the llvm-ir test
12 ↗(On Diff #340369)

Hey @david-arm,
I hope I've addressed your comment.
I've run:
../llvm/utils/ --opt=./bin/opt ../llvm/test/Transforms/LoopVectorize/sve-reduction-inloop.ll and removed some checks that I thought it was not needed.
But I can let the entire output of if you find it better.

david-arm accepted this revision.Apr 27 2021, 12:15 AM

LGTM! Thanks for updating the tests - I think it's a stronger test now with the PHIs and makes it clearer what's going on. :)

10 ↗(On Diff #340561)

nit: Maybe here and in the PHI below it's good to show where TMP34 and TMP36 come from too, i.e. instead of {{%.*}} you can just write vector.body.

14 ↗(On Diff #340561)

nit: It's up to you, but if you prefer a smaller set of CHECK lines you can probably kill off lines TMP21 to TMP25 and just have simple CHECK lines for the loads, i.e.

; CHECK:    [[WIDE_LOAD:%.*]] = load <vscale x 8 x i8>, <vscale x 8 x i8>*

and lower down

; CHECK;  [[WIDE_LOAD2:%.*]] = load <vscale x 8 x i8>, <vscale x 8 x i8>*
This revision is now accepted and ready to land.Apr 27 2021, 12:15 AM
  • address reviewer's comment about the test
CarolineConcatto marked 2 inline comments as done.Apr 30 2021, 6:33 AM

Hi @david-arm,
I have addressed your comments.
I also moved the test to AArch64 folder. I believe is the correct folder for it.

fhahn added inline comments.Apr 30 2021, 6:38 AM
2 ↗(On Diff #341875)

Can this test be target independent by using -force-target-supports-scalable-vectors or does it contain any AArch64 cost-modeling?

41 ↗(On Diff #341875)

might be helpful to use a nicer name for the loop block to make the test a bit easier to parse, maybe %loop? Same for ._crit_edge, perhaps %exit?

david-arm accepted this revision.Apr 30 2021, 6:38 AM

LGTM! Thanks for making the changes. :)

15 ↗(On Diff #341875)

nit: I think you can also just remove lines TMP22-24 as they're unused now.

-Update the test to use -force-target-supports-scalable-vectors
-Move and rename test to scalable-reduction-inloop.ll
-Remove unnecessary checks

2 ↗(On Diff #341875)

Thank you @fhahn for that.
I don't see direct dependency in this test and the fix with the cost model, but I could be wrong.
I've added the flag as you suggested and it works fine.
So I've moved the test to be outside AArch64.
Is that ok?

15 ↗(On Diff #341875)

Thank you @david-arm,
for some reason before this test was failing without these lines, but now it is passing. Probably I was doing something wrong.

41 ↗(On Diff #341875)

Hey @fhahn thank you for your input,
I have changed to loop and ._crit_edge to exit.
But just for reference, this test has the same llvm-ir as in reduction-inloop.ll but now using scalable vectors instead of fixed vector.

david-arm accepted this revision.May 5 2021, 12:33 AM

LGTM! Thanks for making the changes - seems like all review comments have been addressed. :)

fhahn accepted this revision.May 5 2021, 1:50 PM

Thanks for the updates! LGTM

41 ↗(On Diff #341875)

Thanks for improving it!