[PATCH] D148841: [LV] Use SCEV for uniformity analysis across VF

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:2590-2592
+    if (CannotAnalyze)
+      return S;
+    if (SE.isLoopInvariant(S, TheLoop))
----------------
Ayal wrote:
> 
Merged, thanks!


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:2610
+
+  bool canAnalyze() const { return FoundUDiv && !CannotAnalyze; }
+};
----------------
Ayal wrote:
> fhahn wrote:
> > Ayal wrote:
> > > nit: can return SCEVCouldNotCompute (or null) at the end if not FoundUDiv.
> > Unfortunately that doesn't work without additional work, as the returned value may be used to construct the parent SCEV but the rewriter.
> > 
> ok, this is fine.
> 
> I see SCEVInitRewriter, SCEVPostIncRewriter, SCEVShiftRewriter indeed also record a similar SeenLoopVariantSCEVUnknown/Valid which their `rewrite()` queries after visit() to return SCEVCouldNotCompute. Worth doing the same, wrapping constructor/visit()/canAnalyze()?
Updated, thanks!


================
Comment at: llvm/test/Transforms/LoopVectorize/X86/uniform_mem_op.ll:427
+; CHECK-NEXT:    [[TMP39]] = add <4 x i32> [[VEC_PHI6]], [[TMP35]]
+; CHECK-NEXT:    [[INDEX_NEXT]] = add nuw i64 [[INDEX]], 16
+; CHECK-NEXT:    [[VEC_IND_NEXT]] = add <4 x i64> [[STEP_ADD2]], <i64 4, i64 4, i64 4, i64 4>
----------------
Ayal wrote:
> fhahn wrote:
> > Ayal wrote:
> > > note: VF=4 is mandated, previous UF=2 decision with 8 loads is now UF=4 with 4 (uniform) loads & broadcasts.
> > > 
> > > The load from test_base[i/8] could further fold two 'parts' of VF=4 together, as it is uniform across VF=8 / VF=4,UF=2.
> > > Worth leaving behind some **assume**, if not folding directly? I.e., record the largest VF for which uniformity was detected, even if a smaller VF is selected.
> > > Worth optimizing the analysis across VF's, i.e., if a value is known not to be uniform for VF avoid checking for 2*VF? OTOH, if a value is known to be uniform for VF, check only two SCEVs for 2*VF?
> > > The load from test_base[i/8] could further fold two 'parts' of VF=4 together, as it is uniform across VF=8 / VF=4,UF=2.
> > 
> > I think that would be good as follow-up.
> > 
> > > Worth optimizing the analysis across VF's, i.e., if a value is known not to be uniform for VF avoid checking for 2*VF
> > 
> > I was thinking about evaluating something like that as follow-up optimization. WDYT?
> Sure, TODOs can be added to record potential follow-ups.
> 
> Note also that uniformity could be improved beyond comparing equal SCEV expressions, by using Divergence Analysis which propagates uniformity also through uniform branches.
Added a TOOD, thanks!


================
Comment at: llvm/test/Transforms/LoopVectorize/uniform_across_vf_induction1.ll:1
-; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 2
 ; RUN: opt -passes=loop-vectorize -force-vector-interleave=1 -force-vector-width=2 %s -S | FileCheck %s
----------------
Ayal wrote:
> line dropped intentionally?
No that was an accident, added back, thanks!


================
Comment at: llvm/test/Transforms/LoopVectorize/uniform_across_vf_induction2.ll:9
-; CHECK-SAME: (ptr noalias [[A:%.*]], ptr noalias [[B:%.*]]) {
-; ; VF2-LABEL: define void @ld_div1_step1_start0_ind2
 ; VF2-SAME: (ptr noalias [[A:%.*]], ptr noalias [[B:%.*]]) {
----------------
Ayal wrote:
> lines changed intentionally?
Those were left over from the patch that added new run lines, removed in fcc135a8d6a7.


================
Comment at: llvm/test/Transforms/LoopVectorize/uniform_across_vf_induction2.ll:153
-; CHECK-LABEL: define void @ld_div2_step1_start0_ind2
-; CHECK-SAME: (ptr noalias [[A:%.*]], ptr noalias [[B:%.*]]) {
 ; VF2-LABEL: define void @ld_div2_step1_start0_ind2
----------------
Ayal wrote:
> lines dropped intentionally?
Those were left over from the patch that added new run lines, removed in fcc135a8d6a7.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148841/new/

https://reviews.llvm.org/D148841