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

[David Blaikie via llvm-commits]

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.

>
> 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