Skip to content

Rename INS_FLGAS in consistent style for xarch emitter. - #131835

Open
Ruihan-Yin wants to merge 2 commits into
dotnet:mainfrom
Ruihan-Yin:apx-flag-rename
Open

Rename INS_FLGAS in consistent style for xarch emitter.#131835
Ruihan-Yin wants to merge 2 commits into
dotnet:mainfrom
Ruihan-Yin:apx-flag-rename

Conversation

@Ruihan-Yin

Copy link
Copy Markdown
Member

Fix #128266

This PR implements the short-term naming cleanup by standardizing the xarch instruction flags on the INS_FLAGS_* convention.

Converting insFlags to an enum class would require broader changes to shared helper signatures and the ARM code paths, where insFlags has a different purpose. Given the current release-cycle stage, that refactoring is deferred to a larger .NET 12 change.

Copilot AI review requested due to automatic review settings August 4, 2026 22:06
@github-actions github-actions Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 4, 2026
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 5 pipeline(s).
11 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label Aug 4, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Renames a set of xarch insFlags instruction-flag constants to a standardized INS_FLAGS_* naming convention and updates all in-repo call sites accordingly.

Changes:

  • Renamed several xarch insFlags enum values in instr.h (x87 / AVX / APX / pseudo-name flags, plus related masks).
  • Updated flag checks in instr.cpp and emitxarch.cpp to use the new identifiers.
  • Updated xarch instruction metadata tables (instrsxarch.h) to reference the renamed flags.

Reviewed changes

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

File Description
src/coreclr/jit/instr.h Renames xarch insFlags constants and related masks in the enum definition.
src/coreclr/jit/instr.cpp Updates instIsFP / instHasPseudoName to use renamed flags.
src/coreclr/jit/emitxarch.cpp Updates EVEX/APX and AVX helper predicates to use renamed flags.
src/coreclr/jit/instrsxarch.h Updates instruction metadata flag expressions to use renamed flags.

Comment thread src/coreclr/jit/instr.h Outdated
Copilot AI review requested due to automatic review settings August 4, 2026 23:43

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/coreclr/jit/instr.h:247

  • INS_FLAGS_ApxEvexMask introduces mixed-case acronym spelling (Apx/Evex) while nearby flags consistently use all-caps acronyms (e.g., Encoding_EVEX, Encoding_EVEX_APX_ONLY, Encoding_REX2, INS_FLAGS_IsDstDstSrcAVXInstruction). Since this PR’s goal is naming consistency, consider keeping acronyms consistently capitalized (e.g., INS_FLAGS_APX_EVEX_Mask) to match the established style in this enum.
    // Mask of all APX-EVEX related flags. An instruction matches this mask if it either only exists in the
    // APX EVEX space (Encoding_EVEX_APX_ONLY, e.g. crc32_apx/movbe_apx) or gains an APX encoding through the
    // NDD (INS_FLAGS_HasNDD) or NF (INS_FLAGS_HasNF) features. Use it to quickly test whether an instruction
    // has any APX-EVEX capability.
    INS_FLAGS_ApxEvexMask = (Encoding_EVEX_APX_ONLY | INS_FLAGS_HasNDD | INS_FLAGS_HasNF),

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 5 pipeline(s).
11 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@Ruihan-Yin

Copy link
Copy Markdown
Member Author

Hi @JulieLeeMSFT @tannergooding

This change would close #128266 with a less impactful approach.

For the long-term solution, I suggest tracking the refactoring to enum class insFlags in a separate issue, as it touches code across multiple platforms, and targeting it for a future cycle. This keeps the current fix scoped while preserving visibility of the broader cleanup effort.

Please let me know if you have a different idea or approach in mind.

@JulieLeeMSFT

JulieLeeMSFT commented Aug 6, 2026

Copy link
Copy Markdown
Member

LGTM.
Just one question from LLM: The TARGET_AMD64-guarded regions (e.g. emitxarch.cpp:2340 CCMP/CTEST, IsApxExtendedEvexInstruction at 548-558) compile only under AMD64, so an x86-only CI leg would not catch a stale reference inside an AMD64 #ifdef. A clean AMD64 build + an unchanged SPMI asm diff is sufficient proof; just confirming if done.
@dhartglassMSFT, please follow up to merge this PR.

@JulieLeeMSFT JulieLeeMSFT added this to the 11.0.0 milestone Aug 6, 2026
@JulieLeeMSFT JulieLeeMSFT added the Priority:1 Work that is critical for the release, but we could probably ship without label Aug 6, 2026
@Ruihan-Yin

Copy link
Copy Markdown
Member Author

LGTM. Just one question from LLM: The TARGET_AMD64-guarded regions (e.g. emitxarch.cpp:2340 CCMP/CTEST, IsApxExtendedEvexInstruction at 548-558) compile only under AMD64, so an x86-only CI leg would not catch a stale reference inside an AMD64 #ifdef. A clean AMD64 build + an unchanged SPMI asm diff is sufficient proof; just confirming if done. @dhartglassMSFT, please follow up to merge this PR.

Yes, the x64 pipeline and my local AMD64 build & tests passed, confirming there are no stale references in AMD64-only code, primarily involving APX flags. The Windows x64 SuperPMI asm diff also reported no differences.

@Ruihan-Yin Ruihan-Yin changed the title Rename INS_FLGAS in consistent style for x86 emitter. Rename INS_FLGAS in consistent style for xarch emitter. Aug 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI community-contribution Indicates that the PR has been added by a community member Priority:1 Work that is critical for the release, but we could probably ship without

Projects

None yet

Development

Successfully merging this pull request may close these issues.

JIT: Rename insFlags in a consistent style

3 participants