[PATCH] D105473: [LV] Ignore candidate VFs with invalid costs.

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1641-1642
+  /// such instructions are captured in \p Invalid.
+  bool hasInvalidCosts(ElementCount VF,
+                       SmallVectorImpl<Instruction *> *Invalid = nullptr);
+
----------------
There's only one use of hasInvalidCosts, and it doesn't use Invalid. Is it worth just using expectedCost directly?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6049
     // evaluation.
     ChosenFactor.Cost = std::numeric_limits<InstructionCost::CostType>::max();
   }
----------------
Can this use InstructionCost::max() now?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6105-6106
+         << " because of instructions with invalid cost:\n";
+      for (const auto *I : InvalidCosts)
+        OS << "\t" << *I << "\n";
+      OS.flush();
----------------
sdesmalen wrote:
> dmgreen wrote:
> > Do we usually add llvm instructions to optimization reports? Or do they usually use debug info for anything they print?
> It is not common, but there is certainly precedent for it. For example:
> 
> llvm/test/CodeGen/AArch64/GlobalISel/arm64-fallback.ll:
>   ; FALLBACK-WITH-REPORT-ERR: remark: <unknown>:0:0: unable to legalize instruction: G_STORE %3:_(<3 x s32>), %4:_(p0) :: (store 12 into %ir.addr + 16, align 16, basealign 32) (in function: odd_vector)                                                                                                              
> 
> llvm/test/CodeGen/X86/fast-isel-abort-warm.ll:
>   ; CHECK: remark: <unknown>:0:0: FastISel missed call:   call void asm sideeffect
OK. It would simplify it a bit to not have to record the "invalid" instructions.

Do we usually add opt remarks for the costs at each factor? It sounds useful to be honest, but I don't see it anywhere.

Can we split the opt remarks out into a separate patch, to keep this first one simpler.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8040-8041
+    if (!CM.hasInvalidCosts(UserVF)) {
+      LLVM_DEBUG(dbgs() << "LV: Using "
+                        << "user VF " << UserVF << ".\n");
+      CM.collectInLoopReductions();
----------------
sdesmalen wrote:
> dmgreen wrote:
> > Reflow comment.
> > 
> > Will this work for reductions if it's placed here? Can an inloop reduction have an invalid cost? Or are they OK because there is a target hook for them?
> It would, although at the moment the code in getInstructionCost seems to ignore the `Invalid` returned by `getReductionPatternCost`, so that will need a bit of work. It's fine though, since the `canVectorizeReductions` function (which calls a TTI hook) covers this case already.
Oh yeah, that function could probably do with separating out the "did not find a pattern" vs an invalid cost.

Still reflow the comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105473