Page MenuHomePhabricator

[LoopVectorize] Added address space check when analysing interleaved accesses
ClosedPublic

Authored by Ka-Ka on Feb 8 2017, 6:21 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

Ka-Ka created this revision.Feb 8 2017, 6:21 AM
mssimpso edited edge metadata.Feb 8 2017, 8:07 AM

Makes sense. I have comment about the test, though. Thanks!

test/Transforms/LoopVectorize/AArch64/pr31900.ll
1 ↗(On Diff #87640)

I would prefer that we actually check the resulting IR rather than checking for "no crash". Maybe force vectorization (-force-vector-width) and then check that the loads are scalarized.

mkuper added inline comments.Feb 8 2017, 10:12 AM
lib/Transforms/Vectorize/LoopVectorize.cpp
5856 ↗(On Diff #87640)

Hold on.
Shouldn't this fail the distance check? How can we have pointers in different address spaces where we succeed in computing the distance?

mkuper added inline comments.Feb 8 2017, 10:18 AM
lib/Transforms/Vectorize/LoopVectorize.cpp
5856 ↗(On Diff #87640)

Err, sorry, actually went ahead and read the bug. That was the wrong question to ask.

The right question:
Do getMinusSCEV/getAddExpr require pointers to be in the same address space, as an API requirement? I mean, are they supposed to assert, or should they return something sane? Just trying to figure out whether this should be fixed on the caller side or the callee side.
(This code is a good idea anyway, no point in wasting SCEV's time when we know we won't get a constant distance...)

efriedma added inline comments.
lib/Transforms/Vectorize/LoopVectorize.cpp
5856 ↗(On Diff #87640)

SCEV getAddExpr doesn't actually check the address space explicitly, but the operation doesn't make sense unless all the operands have the same size.

Ka-Ka added a subscriber: llvm-commits.
Ka-Ka added inline comments.
lib/Transforms/Vectorize/LoopVectorize.cpp
5856 ↗(On Diff #87640)

Do you want me to try to add address space checks in getMinusSCEV/getAddExpr as well?
Is sounds like it is a larger task to add address space checks in ScalarEvolution. Is is better to do this in a separate review/commit?

test/Transforms/LoopVectorize/AArch64/pr31900.ll
1 ↗(On Diff #87640)

Sure, I will update the testcase in the next patch.

Ka-Ka updated this revision to Diff 87772.Feb 9 2017, 12:12 AM
Ka-Ka marked 2 inline comments as done.

Updated the testcase to use FileCheck.

This looks good to me (aside from a very minor nit about the test run line). Please give Michael and Eli a chance to respond to your questions, though.

test/Transforms/LoopVectorize/AArch64/pr31900.ll
1 ↗(On Diff #87640)

For the run line, we don't need the "-o -", and we now usually input from stdin like "< %s". I think this may make testing faster, but I'm not sure.

mkuper edited edge metadata.Feb 10 2017, 2:36 PM

This LGTM to me too, in the sense that I think this check is a good idea here regardless of what SCEV does.
I don't understand SCEV well enough to say whether we need a change of that side, and I didn't quite understand Eli's point.
In any case, I think we can talk about that in post-commit review if there's anything to talk about, but please give Eli some time to object.

Sorry I wasn't clear. You should check the address spaces of pointers before passing them to getMinusSCEV because the operands of getMinusSCEV are required to have the same bit-width. So this patch is correct as-is.

mkuper accepted this revision.Feb 10 2017, 2:52 PM

Sorry I wasn't clear. You should check the address spaces of pointers before passing them to getMinusSCEV because the operands of getMinusSCEV are required to have the same bit-width. So this patch is correct as-is.

Pointers in different address spaces may or may not have different bitwidths. Will getMinusSCEV assert if the pointers are in different address spaces but have the same bitwidth? And what's the right behavior?

In any case, that's mostly academic. This LGTM.

This revision is now accepted and ready to land.Feb 10 2017, 2:52 PM

The general rule is that SCEV doesn't know anything about pointers; it just handles them as if you converted them to integers using ptrtoint.

Ka-Ka updated this revision to Diff 88195.Feb 13 2017, 6:31 AM
Ka-Ka marked an inline comment as done.

Updated the testcase "run line"

mssimpso accepted this revision.Feb 13 2017, 8:03 AM
This revision was automatically updated to reflect the committed changes.
Ka-Ka added a comment.Feb 14 2017, 5:26 AM

I had to revert this change due to a buildbot failure. I'm trying to recreate the fault locally ...