[llvm] 267ff79 - [SLP] limit verifyFunction to debug build (PR48689)

[Roman Lebedev via llvm-commits]

On Fri, Jan 8, 2021 at 4:55 PM Sanjay Patel <spatel at rotateright.com> wrote:
>
>
>
> On Fri, Jan 8, 2021 at 8:43 AM Nikita Popov <nikita.ppv at gmail.com> wrote:
>>
>> On Fri, Jan 8, 2021 at 2:31 PM Sanjay Patel via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>>>
>>>
>>>
>>> On Fri, Jan 8, 2021 at 8:19 AM Roman Lebedev <lebedev.ri at gmail.com> wrote:
>>>>
>>>> On Fri, Jan 8, 2021 at 4:10 PM Sanjay Patel via llvm-commits
>>>> <llvm-commits at lists.llvm.org> wrote:
>>>> >
>>>> >
>>>> > Author: Sanjay Patel
>>>> > Date: 2021-01-08T08:10:17-05:00
>>>> > New Revision: 267ff7901c745dc903d55599240464ebc4c0bda3
>>>> >
>>>> > URL: https://github.com/llvm/llvm-project/commit/267ff7901c745dc903d55599240464ebc4c0bda3
>>>> > DIFF: https://github.com/llvm/llvm-project/commit/267ff7901c745dc903d55599240464ebc4c0bda3.diff
>>>> >
>>>> > LOG: [SLP] limit verifyFunction to debug build (PR48689)
>>>> >
>>>> > As noted in PR48689, the verifier may have some kind
>>>> > of exponential behavior that should be addressed
>>>> > separately. For now, only run it in debug mode to
>>>> > prevent problems for release+asserts.
>>>> > That limit is what we had before D80401, and I'm
>>>> > not sure if there was a reason to change it in that
>>>> > patch.
>>>> >
>>>> > Added:
>>>> >
>>>> >
>>>> > Modified:
>>>> >     llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
>>>> >
>>>> > Removed:
>>>> >
>>>> >
>>>> >
>>>> > ################################################################################
>>>> > diff  --git a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
>>>> > index 8c06e29341ad..ef0dea0f11d3 100644
>>>> > --- a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
>>>> > +++ b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
>>>> > @@ -2499,7 +2499,7 @@ BoUpSLP::~BoUpSLP() {
>>>> >             "trying to erase instruction with users.");
>>>> >      Pair.getFirst()->eraseFromParent();
>>>> >    }
>>>> > -  assert(!verifyFunction(*F, &dbgs()));
>>>> > +  LLVM_DEBUG(verifyFunction(*F));
>>>> Nitpick: `assert()` already limited the code to debug build only (well,
>>>> -DLLVM_ENABLE_ASSERTIONS=ON) Wrapping it into LLVM_DEBUG()
>>>> made it run wherever `-mllvm -debug-only=SLP` is specified.
>>>
>>>
>>> Yes. IIUC, there's a (potentially large) fraction of LLVM users that build as release with asserts and expect release-like behavior (good perf), so we want to maintain that mode of operation. Once we go to "full" debug build, we give up on any hope of having a useful experience for anything but full-on (extremely slow) debugging.
>>
>>
>> Shouldn't this go under EXPENSIVE_CHECKS, rather than making it debug-only?
>
>
> Hmm, yes that's another option. I don't have a good sense of what the common pattern is. Should all calls to verifyFunction require EXPENSIVE_CHECKS (if so, we would put the ifdef inside the call)?
`verifyFunction()` most certainly should *NOT* be a NOP when
EXPENSIVE_CHECKS is not on, so absolutely not.
Perhaps that whole verifyFunction() call should simply be dropped?