[PATCH] D68814: [LV] Allow assume calls in predicated blocks.

Mon Jan 6 10:44:44 PST 2020

fhahn marked 2 inline comments as done.
fhahn added a comment.

In D68814#1804951 <https://reviews.llvm.org/D68814#1804951>, @Ayal wrote:

> Thanks. Perhaps it's better for Legal to declare via getConditionalAssumes() what is being returning, and let LVP decide what to do with them(*), rather than to assume from the start via getAssumesToDrop() that they must all be dropped.
>
> LVP could collect all conditional assumes itself, btw, but it's easier for Legal to do so, and also logical as they deserve special attention in terms of legality.
>
> (*) As discussed, instead of dropping such assumes could be retained if their condition is retained (being uniform or if handled via isScalarWithPredication()/sinkScalarOperands()?) or if their condition is fused into the assumption.

================
Comment at: llvm/test/Transforms/LoopVectorize/predicate-assume.ll:2
+; REQUIRES: asserts
+; RUN: opt -loop-vectorize -force-vector-width=4 -debug -S %s 2>&1 | FileCheck %s
+
----------------
gilr wrote:
> fhahn wrote:
> > I've not added a test with calls to assume in the loop header here, because we already have them in llvm/test/Transforms/LoopVectorize/X86/assume.ll.
> It would be better to add the new predicated cases to the existing assume.ll.
> (Not sure why it resides under X86 - seems it could instead be target independent and force vectorization, but that can be done separately)
I am not sure. I personally prefer smaller, more targeted test files, but if you think having a larger test file is better I'll merge them.

================
Comment at: llvm/test/Transforms/LoopVectorize/predicate-assume.ll:10
+
+; CHECK: digraph VPlan {
+; CHECK:  N0 [label =
----------------
gilr wrote:
> gilr wrote:
> > Enough to CHECK for VPlan and then CHECK-NOT for the assert?
> ping?
Ah sorry, I totally missed that. I agree it's better to just check for assumes. The autogenerated checks are unnecessary verbose.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68814