[llvm] b01f499 - [NFC][Verifier] Split token1.ll into two, assert/non-assert versions

[Roman Lebedev via llvm-commits]

On Tue, May 4, 2021 at 2:08 AM David Blaikie <dblaikie at gmail.com> wrote:
>
> On Mon, May 3, 2021 at 12:42 AM Nikita Popov <nikita.ppv at gmail.com> wrote:
> >
> > On Mon, May 3, 2021 at 8:40 AM Roman Lebedev via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> >>
> >> Doesn't verifier run on the fully constructed IR?
> >> I.e. won't it always run after said constructor has run?
> >> I'm not really sure how we can perform that verifier check earlier?
> >>
> >> Roman
> >
> >
> > I think you need to either drop the assert, or check this condition already in LLParser, maybe somewhere around https://github.com/llvm/llvm-project/blob/1f8963c80195aa86d02e81acedcf1ff3da127842/llvm/lib/AsmParser/LLParser.cpp#L7175. And possibly the Bitcode parser as well?
>
> Sounds right/like the right direction to me.
My point being, iff we do that *instead* of verifier check,
then we will no longer have a safety net for when the LLVM
is compiled without assertions, and some transform creates such a PHI.

So i'm still a little bit confused.

Roman

> >
> > Nikita
> >
> >>
> >> On Sun, May 2, 2021 at 10:46 PM David Blaikie <dblaikie at gmail.com> wrote:
> >> >
> >> > On Sun, May 2, 2021 at 12:34 PM Roman Lebedev <lebedev.ri at gmail.com> wrote:
> >> >>
> >> >> Originally we had a verifier check for phis with token type,
> >> >> i made the PHINode constructor assert if the type is a token,
> >> >> which obviously broke the verifier test.
> >> >
> >> >
> >> > Sounds like the verifier may need to be changed to check this condition earlier, then? We shouldn't be able to reach/fire an assertion on any input when running the verifier, I think.
> >> >
> >> >>
> >> >> If we drop the test for what happens without assertions,
> >> >> that implies we no longer care what the verifier does
> >> >> about such PHI's. Is that really so?
> >> >>
> >> >> Roman
> >> >>
> >> >> On Sun, May 2, 2021 at 10:11 PM David Blaikie <dblaikie at gmail.com> wrote:
> >> >> >
> >> >> > Could you explain the purpose of these two tests a bit more?
> >> >> >
> >> >> > In general I don't think we should be testing the behavior of assertions except in pretty narrow cases (like checking that the Error API asserts in certain ways that are critical to its usability) - since they represent essentially "undefined behavior".
> >> >> >
> >> >> > And if we are testing the behavior of an assertion, we shouldn't be testing what happens without assertions - because, again, if an assertion can be triggered then the behavior "behind" the assert is undefined, we shouldn't expect any particular behavior. (if we expect particular behavior, we shouldn't assert)
> >> >> >
> >> >> > On Wed, Apr 28, 2021 at 3:59 AM Roman Lebedev via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> >> >> >>
> >> >> >>
> >> >> >> Author: Roman Lebedev
> >> >> >> Date: 2021-04-28T13:58:38+03:00
> >> >> >> New Revision: b01f499861235f414f6740bbf950d095bceada4d
> >> >> >>
> >> >> >> URL: https://github.com/llvm/llvm-project/commit/b01f499861235f414f6740bbf950d095bceada4d
> >> >> >> DIFF: https://github.com/llvm/llvm-project/commit/b01f499861235f414f6740bbf950d095bceada4d.diff
> >> >> >>
> >> >> >> LOG: [NFC][Verifier] Split token1.ll into two, assert/non-assert versions
> >> >> >>
> >> >> >> Added:
> >> >> >>     llvm/test/Verifier/token1-with-asserts.ll
> >> >> >>     llvm/test/Verifier/token1-without-asserts.ll
> >> >> >>
> >> >> >> Modified:
> >> >> >>
> >> >> >>
> >> >> >> Removed:
> >> >> >>     llvm/test/Verifier/token1.ll
> >> >> >>
> >> >> >>
> >> >> >> ################################################################################
> >> >> >> diff  --git a/llvm/test/Verifier/token1.ll b/llvm/test/Verifier/token1-with-asserts.ll
> >> >> >> similarity index 100%
> >> >> >> rename from llvm/test/Verifier/token1.ll
> >> >> >> rename to llvm/test/Verifier/token1-with-asserts.ll
> >> >> >>
> >> >> >> diff  --git a/llvm/test/Verifier/token1-without-asserts.ll b/llvm/test/Verifier/token1-without-asserts.ll
> >> >> >> new file mode 100644
> >> >> >> index 000000000000..ef2d25c045d2
> >> >> >> --- /dev/null
> >> >> >> +++ b/llvm/test/Verifier/token1-without-asserts.ll
> >> >> >> @@ -0,0 +1,12 @@
> >> >> >> +; REQUIRES: !asserts
> >> >> >> +; RUN: not llvm-as %s -o /dev/null 2>&1 | FileCheck %s
> >> >> >> +
> >> >> >> +define void @f(token %A, token %B) {
> >> >> >> +entry:
> >> >> >> +  br label %bb
> >> >> >> +
> >> >> >> +bb:
> >> >> >> +  %phi = phi token [ %A, %bb ], [ %B, %entry]
> >> >> >> +; CHECK: PHI nodes cannot have token type!
> >> >> >> +  br label %bb
> >> >> >> +}
> >> >> >>
> >> >> >>
> >> >> >>
> >> >> >> _______________________________________________
> >> >> >> llvm-commits mailing list
> >> >> >> llvm-commits at lists.llvm.org
> >> >> >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> >> _______________________________________________
> >> llvm-commits mailing list
> >> llvm-commits at lists.llvm.org
> >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits