[PATCH] D106843: [IVDescriptors] Fix bug in checkOrderedReduction

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/test/Transforms/LoopVectorize/fp-reduction-crash.ll:1
+; REQUIRES: asserts
+; RUN: opt < %s -loop-vectorize -S | FileCheck %s
----------------
anna wrote:
> fhahn wrote:
> > fhahn wrote:
> > > anna wrote:
> > > > kmclaughlin wrote:
> > > > > nit: I don't think `REQUIRES: asserts` is necessary for this test?
> > > > This is to show that the failure only occurs in assertions enabled mode. It will not fail in any other mode.
> > > > 
> > > > To put it differently, if I first landed this testcase alone, it will succeed in non-assertion enabled mode and it isn't clear what the problem is (since we cannot add `XFAIL`). 
> > > > 
> > > Can you add check lines to make sure the IR is transformed as expected? That way, we do not need to rely on the asserting causing the test to crash.
> > actually, can the test be added to the file already containing order reduction tests, with a comment explaining what it checks? That will help make maintaining it easier.
> The reason it doesn't cause any failure in non-assertion mode is that this just checks and bails out without doing vectorization. The bail out was too late. That was the only reason this crash occurred. 
> So, the IR is not transformed for this reduction pattern - not sure what the CHECK-LINES will do. I will add a comment explaining what it checks and move it to the order-reduction tests.
> So, the IR is not transformed for this reduction pattern - not sure what the CHECK-LINES will do. I will add a comment explaining what it checks and move it to the order-reduction tests.

Sounds good to me, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106843