Page MenuHomePhabricator

[Clang] Add option to handle behaviour of vector bool/vector pixel.
ClosedPublic

Authored by stefanp on Jun 3 2021, 6:43 AM.

Details

Summary

Added the option -altivec-src-compat=[mixed,gcc,xl]. The default at this time is mixed.

The default behavior for clang is for all vector compares to return a scalar unless the vectors being
compared are vector bool or vector pixel. In that case the compare returns a
vector. With the gcc case all vector compares return vectors and in the xl case
all vector compares return scalars.

This patch does not change the default behavior of clang.

This option will be used in future patches to implement behaviour compatibility for the vector bool/pixel types.

Diff Detail

Event Timeline

stefanp created this revision.Jun 3 2021, 6:43 AM
stefanp requested review of this revision.Jun 3 2021, 6:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2021, 6:43 AM
stefanp added a reviewer: Restricted Project.Jun 3 2021, 6:43 AM
bmahjour added a subscriber: bmahjour.EditedJun 4 2021, 10:52 AM

As far as I can see, there is no good reason for the special treatment of vector bool/pixel going forward. Could we drop this special treatment, or at least change the default to use scalar results across the board (consistent with XL's behaviour and clang's current behaviour for most cases).

clang/include/clang/Driver/Options.td
3826

I'm not sure the term "ABI" is really applicable. Maybe we should call it "vector-compare-compat="

clang/test/CodeGen/vector-compat-pixel-bool-ternary.c
7

I only see the clang FE interface being tested. Does this have to be specified through -Xclang -vector-abi-compat=... or is there a clang driver option for it as well? I think we should have a clang driver option and have at least one test for it.

As far as I can see, there is no good reason for the special treatment of vector bool/pixel going forward. Could we drop this special treatment, or at least change the default to use scalar results across the board (consistent with XL's behaviour and clang's current behaviour for most cases).

We can change this but I am hesitant to make the change immediately. I can leave the default behavior as-is for now and add a warning to say that this feature is going to be deprecated at a later date. After a couple of releases we can then change the default. I don't want to change defaults without giving users some kind of warning first.

As a result of this I'm going to change the name in the enum from Default to Mixed as it does not make sense to have it named Default if it's not going to be default in the long run.

clang/include/clang/Driver/Options.td
3826

Sure. I can change the name to vector-compare-compat.

clang/test/CodeGen/vector-compat-pixel-bool-ternary.c
7

This option works when it is passed immediately to clang.
I will add a couple of RUN lines to test this as well.

stefanp updated this revision to Diff 351467.Jun 11 2021, 9:03 AM

Updated the name of the option to vector-compare compat.
Added clang test lines to the test cases.
Added deprecation warnings to the current default behaviour of vector bool and
vector pixel.

bmahjour added inline comments.Jun 14 2021, 11:24 AM
clang/include/clang/Basic/LangOptions.h
248

How about adding Default = Mixed at the end and moving the comment with it?

249

Pixel -> pixel

250

[nit] Pixel -> pixel

clang/lib/Sema/SemaExpr.cpp
12232

Check for getLangOpts().AltiVec before the switch?

stefanp updated this revision to Diff 353126.Jun 18 2021, 4:39 PM

Moved if statement out of switch and added the Default=Mixed.

bmahjour added a comment.EditedJun 21 2021, 10:00 AM

Sorry I didn't mention this in my earlier comment about the option name, but I think that all inconsistencies in handling vector bool/pixel types should be controlled by a single compatibility option. For example the current special handling of initialization (splat vs non-splat) for these types should also be option controlled. For that reason we should design and name the option in a more generic way. The same consideration should go into the wording of the deprecation warning.
Some suggestions:

rename -vector-compare-compat to -altivec-compatibility or -altivec-compat or -altivec-bool-pixel-compat or -vector-bool-pixel-compat. (I prefer the last two)
reword the message to:
"Current handling of vector bool and vector pixel types in this context are deprecated. The default behaviour will soon change to that implied by the '-altivec-compat=xl' option"

Please also update the description section of this review.

clang/include/clang/Basic/DiagnosticSemaKinds.td
7447

We need to add a new grouping (see InGroup<...>) for this, otherwise users won't be able to turn this warning off individually. There is also a clang LIT test that would fail if a warning message is added without a grouping specified. Please make sure check-all passes.

clang/include/clang/Basic/LangOptions.def
130

VectorCompareCompatKind::Mixed -> VectorCompareCompatKind::Default

I haven't had time to review this yet, but I just wanted to chime in on the option spelling and description. I think we should go with:

-faltivec-src-compat={xl|gcc|mixed}
  Source-level compatibility for Altivec vectors (for PowerPC targets). This
  includes results of vector comparison (scalar for 'xl', vector for 'gcc')
  as well as behavior when initializing with a scalar (splatting for 'xl',
  element zero only for 'gcc'). For 'mixed', the compatibility is as 'gcc' for
  'vector bool/vector pixel' and as 'xl' for other types. Current default is
  'mixed'.

The name makes it clear that this has to do with source compatibility for Altivec vector types. The description is rather verbose - much more so than most options, but I think that is useful for an option such as this - that has a complex set of implications.

stefanp updated this revision to Diff 354422.Jun 24 2021, 7:36 PM

Updated the name of the option to cover all of the inconsistencies for vector
pixel/bool. Future patches will continue to use this option to define the
behaviour of these two types.

Reworded the warning message.
Added InGroup<> to the warning.
Set the default to Default.

stefanp retitled this revision from [Clang] Add option for vector compare compatibility. to [Clang] Add option to handle behaviour of vector bool/vector pixel..Jun 24 2021, 7:39 PM
stefanp edited the summary of this revision. (Show Details)
stefanp edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Jun 25 2021, 10:21 AM

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.

dyung added a subscriber: dyung.Jun 28 2021, 11:03 PM

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.

Our internal bot also caught it and I was readying a patch to add "REQUIRES: asserts" to the tests as a workaround.

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.

Our internal bot also caught it and I was readying a patch to add "REQUIRES: asserts" to the tests as a workaround.

Thanks for the confirmation! Yeah, I probably could've/should've settled for that narrower solution - good to keep in mind for next time.

stefanp reopened this revision.Jun 29 2021, 6:59 AM

I'm sorry I missed the asserts requirement.
I will recommit this patch after I add REQUIRES: asserts.

This revision is now accepted and ready to land.Jun 29 2021, 6:59 AM

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.

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.

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.

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.

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

Agreed.

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.

Right.

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.

Do you happen to know what that option is? Thanks!

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

Agreed.

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.

Right.

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.

Do you happen to know what that option is? Thanks!

Generally called "discard value names" - clang has -f{no-}discard-value-names - the various command line tools (opt, llc, etc) have some similar flags with spelling more suitable to the various command line syntaxes, etc...

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.

I've removed all of the entry: checks and changed for next line to CHECK: and that seems to be working with assertions off. I will recommit the patch like that if this is preferred.

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.

I've removed all of the entry: checks and changed for next line to CHECK: and that seems to be working with assertions off. I will recommit the patch like that if this is preferred.

Sounds alright to me

stefanp updated this revision to Diff 355315.Jun 29 2021, 11:53 AM

Updated test cases to avoid failures on non assert builds.

stefanp updated this revision to Diff 355320.Jun 29 2021, 11:58 AM

Removed the lines that specified that checks were auto generated.

This revision was landed with ongoing or failed builds.Jun 29 2021, 12:07 PM
This revision was automatically updated to reflect the committed changes.

I'm unfamiliar with the altivec API. What's a reasonable source code update that preserves the current default behavior -altivec-src-compat=mixed under -altivec-src-compat=xl, i.e., under -altivec-src-compat=xl how would we compare vector bool or vector pixel to produce a vector?

I'm unfamiliar with the altivec API. What's a reasonable source code update that preserves the current default behavior -altivec-src-compat=mixed under -altivec-src-compat=xl, i.e., under -altivec-src-compat=xl how would we compare vector bool or vector pixel to produce a vector?

For vector bool comparison one could use vec_cmpeq and vec_compne (see https://www.ibm.com/docs/en/xl-c-and-cpp-linux/16.1.1?topic=functions-vec-cmpeq). However, vec_cmp* interfaces do not currently support vector pixel. Do you have a concrete use case for comparing vector pixels? If so I think we should consider adding corresponding overloads to altivec.h.

I'm unfamiliar with the altivec API. What's a reasonable source code update that preserves the current default behavior -altivec-src-compat=mixed under -altivec-src-compat=xl, i.e., under -altivec-src-compat=xl how would we compare vector bool or vector pixel to produce a vector?

For vector bool comparison one could use vec_cmpeq and vec_compne (see https://www.ibm.com/docs/en/xl-c-and-cpp-linux/16.1.1?topic=functions-vec-cmpeq). However, vec_cmp* interfaces do not currently support vector pixel. Do you have a concrete use case for comparing vector pixels? If so I think we should consider adding corresponding overloads to altivec.h.

Thank you! I don't have a use case for comparing vector pixels.