Skip to content

C++: Handle Arm SVE in the IR #19845

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 24, 2025
Merged

C++: Handle Arm SVE in the IR #19845

merged 1 commit into from
Jun 24, 2025

Conversation

jketema
Copy link
Contributor

@jketema jketema commented Jun 23, 2025

Observe that MS calculator is currently broken in DCA. I don't think missing out on the project is critical for this PR. I've opened microsoft/calculator#2347 to hopefully get this resolved.

@github-actions github-actions bot added the C++ label Jun 23, 2025
@jketema jketema marked this pull request as ready for review June 23, 2025 13:56
@Copilot Copilot AI review requested due to automatic review settings June 23, 2025 13:56
@jketema jketema requested a review from a team as a code owner June 23, 2025 13:56
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds support for Arm Scalable Vector Extension (SVE) types in the intermediate representation generation and updates related tests.

  • Renames and extends NEON tests (arm.cpparm_neon.cpp) and adds a new arm_sve.cpp test.
  • Updates expected IR and AST outputs for both NEON and SVE patterns.
  • Treats ScalableVectorType as opaque (size 0) and includes it in const-propagation (isDeeplyConstBelow) rules in the QL IR definitions.

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
cpp/ql/test/library-tests/ir/ir/raw_ir.expected Updated expected raw IR for NEON and added SVE cases
cpp/ql/test/library-tests/ir/ir/aliased_ir.expected Updated aliased IR expectations for NEON and SVE
cpp/ql/test/library-tests/ir/ir/PrintAST.expected Updated AST print expectations for NEON and SVE
cpp/ql/test/library-tests/ir/ir/arm_neon.cpp New NEON test source
cpp/ql/test/library-tests/ir/ir/arm_sve.cpp New SVE test source
cpp/ql/lib/semmle/code/cpp/ir/internal/CppType.qll Treat ScalableVectorType as size-0 in getTypeSizeWorkaround and isOpaqueType
cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/SideEffects.qll Include ScalableVectorType in isDeeplyConstBelow
Comments suppressed due to low confidence (2)

cpp/ql/lib/semmle/code/cpp/ir/internal/CppType.qll:34

  • [nitpick] Add a QLDoc comment above this clause to explain why ScalableVectorType is treated as size 0 (opaque) and when this fallback might need revisiting.
      type instanceof ScalableVectorType and

cpp/ql/lib/semmle/code/cpp/ir/internal/CppType.qll:34

  • Add or extend a unit test to verify that getTypeSizeWorkaround returns 0 for ScalableVectorType, ensuring the new fallback is exercised.
      type instanceof ScalableVectorType and

Comment on lines 55 to +58
isDeeplyConst(t.(GNUVectorType).getBaseType())
or
isDeeplyConst(t.(ScalableVectorType).getBaseType())
or
Copy link
Preview

Copilot AI Jun 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Consider grouping the vector-type const checks (GNUVectorType and ScalableVectorType) into a single reusable predicate to reduce duplication.

Suggested change
isDeeplyConst(t.(GNUVectorType).getBaseType())
or
isDeeplyConst(t.(ScalableVectorType).getBaseType())
or
isVectorType(t) and isDeeplyConst(t.getBaseType())
or

Copilot uses AI. Check for mistakes.

@jketema jketema added the no-change-note-required This PR does not need a change note label Jun 23, 2025
Copy link
Contributor

@IdrissRio IdrissRio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@jketema jketema merged commit 676289e into github:main Jun 24, 2025
17 of 18 checks passed
@jketema jketema deleted the ir-sve branch June 24, 2025 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants