Rename INS_FLGAS in consistent style for x86 emitter. - #131835
Rename INS_FLGAS in consistent style for x86 emitter.#131835Ruihan-Yin wants to merge 2 commits into
Conversation
|
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. |
There was a problem hiding this comment.
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
insFlagsenum values ininstr.h(x87 / AVX / APX / pseudo-name flags, plus related masks). - Updated flag checks in
instr.cppandemitxarch.cppto 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. |
There was a problem hiding this comment.
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_ApxEvexMaskintroduces 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: 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. |
|
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. |
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.