This is an archive of the discontinued LLVM Phabricator instance.

[SVE] Suppress vselect warning from incorrect interface call
ClosedPublic

Authored by nasherm on Mar 9 2021, 4:19 AM.

Details

Summary

The VSelectCombine handler within AArch64ISelLowering,
uses an interface call which only expects fixed vectors.
This generates a warning when the call is made on a
scalable vector. This warning has been suppressed with this change,
by using the ElementCount interface, which supports both fixed and scalable vectors.
I have also added a regression test which recreates the warning.

Diff Detail

Unit TestsFailed

Event Timeline

nasherm created this revision.Mar 9 2021, 4:19 AM
nasherm requested review of this revision.Mar 9 2021, 4:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 9 2021, 4:19 AM

Hi @nasherm, the fix looks good - just a few minor comments!

llvm/include/llvm/CodeGen/ValueTypes.h
301

nit: I think this needs a full stop at the end.

327

nit: It would be good to use up to 80 chars on the first line of the comment in line with the other comments.

llvm/test/CodeGen/AArch64/vselect-sve-interface-warnings.ll
1

The way we've done in this other tests is to combine the codegen with the warnings check, i.e.

; RUN: llc -mtriple=aarch64--linux-gnu -mattr=+sve < %s 2>%t | FileCheck %s
; RUN: FileCheck --check-prefix=WARN --allow-empty %s <%t

whereas here you're only checking for warnings. I think it would be good to check the generated code here too in a similar way and a comment to the test describing how the user can fix any warnings that may fire in this test. Again, we've added this to other tests:

; If this check fails please read test/CodeGen/AArch64/README for instructions on how to resolve it.

Also, the naming convention we've used for other SVE tests is usually something like sve-fcmp.ll. It would be good if you could something similar, e.g. sve-vselect.ll

21

From the code you've fixed it looks like we should go down the same path for all the comparison opcodes so maybe you only need to test one or two cases, e.g. "icmp ne", "icmp sgt", or something like that?

nasherm updated this revision to Diff 329657.Mar 10 2021, 7:24 AM

Response to David's comments

nasherm updated this revision to Diff 331204.Mar 17 2021, 3:32 AM

Slight change to the checking call which triggers the warning. We now
check using the ElementCount interface.

Just two more nits before I'm happy to accept.

llvm/test/CodeGen/AArch64/sve-vselect-interface-warnings.ll
1 ↗(On Diff #331204)

Can you rename the file to sve-cmp-select.ll ?

We're planning to remove the 'warning' mechanism in the future, so it's better not to have warning in the name of files.

7 ↗(On Diff #331204)

Please change the types to <vscale x 16 x i8> (and <vscale x 16 x i1> for the predicate in the select), as that simplifies the resulting code a bit (i.e. no type legalization is necessary then).

nasherm updated this revision to Diff 331554.EditedMar 18 2021, 7:10 AM
nasherm marked an inline comment as done.

Addressed Sander's comments regarding renaming and using <vselect x 16 x ty> types.

nasherm updated this revision to Diff 331806.Mar 19 2021, 2:36 AM

Removed changes to comments within ValueType.h

This revision is now accepted and ready to land.Mar 24 2021, 6:54 AM
This revision was landed with ongoing or failed builds.Mar 24 2021, 7:34 AM
This revision was automatically updated to reflect the committed changes.