[PATCH] D103615: [Clang] Add option to handle behaviour of vector bool/vector pixel.

[David Blaikie via Phabricator via cfe-commits]

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