[PATCH] D94869: [LV] Fix crash when computing max VF too early

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5534-5535
   if (!useMaskedInterleavedAccesses(TTI)) {
     assert(WideningDecisions.empty() && Uniforms.empty() && Scalars.empty() &&
            "No decisions should have been taken at this point");
     // Note: There is no need to invalidate any cost modeling decisions here, as
----------------
c-rhodes wrote:
> @fhahn Any thoughts on this? This assert is firing since D90687, details above. I'm just wondering having looked at your patch D78298 if there's a more sensible fix here, maybe to get rid of the assert and call `invalidateCostModelingDecisions`?
I think it is still preferable to avoid spending time computing cost-model decisions unnecessarily.

Duplicating the MaxVF computation at a few places to ensure we only do it when needed looks good to me.


================
Comment at: llvm/test/Transforms/LoopVectorize/Hexagon/maximum-vf-crash.ll:1
+; RUN: opt -march=hexagon -hexagon-autohvx -loop-vectorize -disable-output < %s
+
----------------
please add some check lines to make sure something sensible happens, besides not crashing.


================
Comment at: llvm/test/Transforms/LoopVectorize/Hexagon/maximum-vf-crash.ll:14
+;
+; int f() {
+;   int g = 0;
----------------
Personally I think the C source code mostly just adds clutter. Ideally the IR source would be concise and with descriptive variable names, so it should be relatively easy to see what's going on without C source; especially if the C source looks like something auto-generated/C-reduced. Also, there's no guarantee that Clang will generate the same IR in future versions.


================
Comment at: llvm/test/Transforms/LoopVectorize/Hexagon/maximum-vf-crash.ll:44
+; Function Attrs: optsize
+define dso_local i32 @f() local_unnamed_addr #0 {
+entry:
----------------
can this function be a bit more simplified & cleaned up? I'll leave some suggestions below & I think the basic block names could be improved & shortened,


================
Comment at: llvm/test/Transforms/LoopVectorize/Hexagon/maximum-vf-crash.ll:46
+entry:
+  %.pr = load i32, i32* @d, align 4
+  %tobool.not15 = icmp eq i32 %.pr, 0
----------------
none of this should be needed to reproduce the failure, you should be able to just use a constant instead of `%.pr` as incoming value below.


================
Comment at: llvm/test/Transforms/LoopVectorize/Hexagon/maximum-vf-crash.ll:51
+for.cond1.preheader.lr.ph:                        ; preds = %entry
+  %0 = load %struct.b*, %struct.b** @c, align 4
+  br label %for.cond1.preheader
----------------
can we instead just pass a pointer argument?


================
Comment at: llvm/test/Transforms/LoopVectorize/Hexagon/maximum-vf-crash.ll:57
+  %1 = phi i32 [ %.pr, %for.cond1.preheader.lr.ph ], [ %inc6, %for.cond1.preheader ]
+  %a10 = getelementptr inbounds %struct.b, %struct.b* %0, i32 %1, i32 0
+  %2 = load i8, i8* %a10, align 1
----------------
instead of using a struct, can this just be plain pointer to `i8` or something like that?


================
Comment at: llvm/test/Transforms/LoopVectorize/Hexagon/maximum-vf-crash.ll:61
+  %conv = zext i8 %2 to i32
+  %3 = icmp ugt i32 %conv, 1
+  %umax = select i1 %3, i32 %conv, i32 1
----------------
are all those compares/extensions/selects needed?


================
Comment at: llvm/test/Transforms/LoopVectorize/Hexagon/maximum-vf-crash.ll:72
+  store i32 %inc4.lcssa18, i32* @e, align 4
+  store i32 0, i32* @d, align 4
+  br label %for.end7
----------------
not needed?


================
Comment at: llvm/test/Transforms/LoopVectorize/Hexagon/maximum-vf-crash.ll:82
+
+!0 = distinct !{!0, !1}
+!1 = !{!"llvm.loop.mustprogress"}
----------------
not needed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94869