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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen 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);
+
----------------
dmgreen wrote:
> There's only one use of hasInvalidCosts, and it doesn't use Invalid. Is it worth just using expectedCost directly?
Could do, but that means making `expectedCost` public instead of private. I wasn't sure if there was a specific reason these cost-queries were private.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6049
     // evaluation.
     ChosenFactor.Cost = std::numeric_limits<InstructionCost::CostType>::max();
   }
----------------
dmgreen wrote:
> Can this use InstructionCost::max() now?
Yes! Good catch. I've made the change in D105113 where it belonged in the first place.


================
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();
----------------
dmgreen wrote:
> 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.
> 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.
Sure will do. Perhaps we can collate some of this information to print a more concise report, without having to print remarks for each factor, for example:

  remark: Not all VFs are feasible due to invalid costs for:
    call @llvm.sin(...) (at VF=vscale x 2)

Or when it cannot use scalable vectors for any of the candidate VFs:

  remark: Could not vectorize with scalable vectors because none of the candidate VFs result in a valid cost:
    mul i64 %reduction, %val (at VF=vscale x 1, vscale x 2, vscale x 4)
    call @llvm.sin(...) (at VF=vscale x 2)




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105473