dblaikie added a comment. In D103615#2847118 <https://reviews.llvm.org/D103615#2847118>, @bmahjour wrote:
> In D103615#2847047 <https://reviews.llvm.org/D103615#2847047>, @stefanp wrote: > >> I'm sorry I missed the asserts requirement. >> I will recommit this patch after I add `REQUIRES: asserts`. > > Instead of disabling the tests for non-assert builds, can we just remove the > `entry:` checks at the beginning of each function? The rest of the IR checks > should pass since they use a regexp so they should match for either named or > unnamed instructions. (generally: disabling the test in non-asserts builds isn't the right path, modifying the test so it doesn't depend on asserts IR naming is the right path) Yes, probably removing the `entry:` check would be sufficient - give it a test locally and see how it goes. (it does mean the "CHECK-NEXT" after that (for the first instruction) would have to be a plain "CHECK" - so that the test could pass both in the presence and absence of the entry label. In D103615#2847124 <https://reviews.llvm.org/D103615#2847124>, @bmahjour wrote: > In D103615#2846245 <https://reviews.llvm.org/D103615#2846245>, @dblaikie > wrote: > >> As mentioned in the reverting commit - these tests fail in non-asserts >> builds, because they assume named IR instructions (like the named entry BB >> label), which aren't provided on a non-asserts build (there's a flag to turn >> them on - but that's probably not the right fix - making the test resilient >> to non-asserts IR is probably the right fix). >> >> Leaving non-asserts builds broken for 12 hours (maybe I'm the first one to >> come across/report it - but I'd expect there are some buildbots that would >> fail, etc) is quite a while - best to be avoided when possible. > > This is a bit off topic, but I'm just curious about clang's rationale for > producing different IR depending on whether asserts are on/off? Seems strange > and undesirable to me. Yeah, seems like a weird choice to me too (though has been around a long time, so folks are pretty used to it) - might be worth bringing it up on llvm-dev. I think we now have a flag to enable this functionality that works even in non-asserts builds (maybe?) so maybe if we just change the default for assert builds so it's always opt-in via a flag, then it's consistent between asserts and non-asserts builds. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103615/new/ https://reviews.llvm.org/D103615 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
