This is an archive of the discontinued LLVM Phabricator instance.

[LV] Adds simple analysis that improves VF-aware uniformity checks.
AbandonedPublic

Authored by vporpo on Apr 6 2023, 12:50 PM.

Details

Reviewers
fhahn
reames
Ayal
Summary

This helps generate broadcasts instead of gathers in cases like:

for (i = 0; i != N; ++i)
  ... = ... B[i/4]

when VF is a multiple of 4.

And will generate a single store in cases like this:

for (i = 0; i != N; ++i)
  A[i/4] = ...

when VF is a multiple of 4.

Without this patch the uniformity checks only test for loop-invariance, so they
return false for i/4, even though i/4 will always return i/4 for all
i/4, (i+1)/4, (i+2)/4 and (i+3)/4.

This patch adds a simple analysis that checks if a value is a linear expression
of the induction variable divided by a constant and that the VF is a multiple
of this constant. If so, the value is treaded as uniform.

Diff Detail

Event Timeline

vporpo created this revision.Apr 6 2023, 12:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2023, 12:50 PM
vporpo requested review of this revision.Apr 6 2023, 12:50 PM
vporpo updated this revision to Diff 511523.Apr 6 2023, 2:08 PM

Fixed build

vporpo added a reviewer: Ayal.Apr 6 2023, 7:42 PM
vporpo updated this revision to Diff 511708.Apr 7 2023, 9:46 AM

No need to match stores in uniformAcrossVF().

vporpo updated this revision to Diff 511722.Apr 7 2023, 10:51 AM

Removed redundant APInt.

vporpo updated this revision to Diff 515434.Apr 20 2023, 12:30 PM

rebase and ping

fhahn added a comment.Apr 20 2023, 2:06 PM

Thanks for sharing the patch! I added some comments inline. Reasoning about this manually seems a bit fragile and verbose so I was wondering if it might be possible to re-use the existing reasoning infrastructure we have in SCEV to do the heavy lifting. I put up a patch that use SCEV, but I might be missing something that would make this unfeasible: D148841

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
488

This seems overly restrictive.

495

Shouldn't this check if Op1 is the induction we will use the step from?

502

Using this API to just get the step value seems unnecessary restrictive. Better to just get the induction descriptor for the induction.

503

IIUC getBounds will only work if IV is the main induction phi, but there could be multiple inductions.

506

I might be missing where this is handled, but doesn't the code also have to account for the start value of the induction?

Thanks for sharing the patch! I added some comments inline. Reasoning about this manually seems a bit fragile and verbose so I was wondering if it might be possible to re-use the existing reasoning infrastructure we have in SCEV to do the heavy lifting. I put up a patch that use SCEV, but I might be missing something that would make this unfeasible: D148841

Cool, thanks! Yeah the SCEV approach is much better. Let me work on the tests so we can test all the different cases.

vporpo abandoned this revision.Apr 25 2023, 1:48 PM

Abandoning since we are going to use the SCEV-based approach: https://reviews.llvm.org/D148841