Skip to content

Remove -Wno-unused-function -Wno-tautological-compare -Wno-unused-value for clrjit projects#128271

Open
EgorBo wants to merge 14 commits into
dotnet:mainfrom
EgorBo:fix-warn
Open

Remove -Wno-unused-function -Wno-tautological-compare -Wno-unused-value for clrjit projects#128271
EgorBo wants to merge 14 commits into
dotnet:mainfrom
EgorBo:fix-warn

Conversation

@EgorBo
Copy link
Copy Markdown
Member

@EgorBo EgorBo commented May 15, 2026

We currently define the followings for the entire native code:

  • add_compile_options(-Wno-unused-variable)
  • add_compile_options(-Wno-unused-value)
  • add_compile_options(-Wno-unused-function)
  • add_compile_options(-Wno-tautological-compare)
  • add_compile_options(-Wno-unknown-pragmas)

my initial attempt to remove them all led to a very big diff so I decided to focus on JIT first.

Copilot AI review requested due to automatic review settings May 15, 2026 22:54
@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 May 15, 2026
@dotnet-policy-service
Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Copy link
Copy Markdown
Contributor

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

Re-enables -Wunused-function for the JIT (by removing the repo-wide -Wno-unused-function for this directory on Unix/WASI) and fixes the warnings that surfaced. This includes guarding several helpers with the same #ifdef conditions as their callers, deleting a truly unused overload, and incidentally repairing two bugs (a missing assignment and a discarded expression) that the warning exposed.

Changes:

  • Enable -Wunused-function for clrjit targets via a new block in src/coreclr/jit/CMakeLists.txt.
  • Guard target/DEBUG-only helpers (RelopEvaluationResultString, TryGetStoreCoalescingConstantBits, MatchIntConstSelectValues/IntConstSelectOper, isLowSimdReg) with matching #ifdefs, and delete the unused CorInfoType-taking isSupportedBaseType overload.
  • Fix two latent bugs uncovered by the warning: a missing id = assignment in GetHWIntrinsicIdForCmpOp (GT_GT/isScalar/xarch path) and discarded impPopStack().val expressions in impIntrinsic.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/coreclr/jit/CMakeLists.txt Strip -Wno-unused-function from the JIT directory's compile options on Unix/WASI.
src/coreclr/jit/scev.cpp Wrap debug-only RelopEvaluationResultString in #ifdef DEBUG.
src/coreclr/jit/lower.cpp Guard TryGetStoreCoalescingConstantBits for TARGET_XARCH/TARGET_ARM64.
src/coreclr/jit/ifconversion.cpp Guard IntConstSelectOper/MatchIntConstSelectValues for TARGET_RISCV64.
src/coreclr/jit/emitxarch.cpp Wrap debug-only isLowSimdReg in #ifdef DEBUG.
src/coreclr/jit/hwintrinsic.cpp Remove unused CorInfoType-overload of isSupportedBaseType; remaining var_types overload is the one used at all call sites.
src/coreclr/jit/gentree.cpp Fix missing id = assignment in scalar GT_GT case in GetHWIntrinsicIdForCmpOp.
src/coreclr/jit/importercalls.cpp Replace impPopStack().val; no-op statements with impPopStack();.

Copilot AI review requested due to automatic review settings May 15, 2026 23:08
@EgorBo EgorBo changed the title Remove -Wno-unused-function for clrjit projects Remove -Wno-unused-function -Wno-tautological-compare -Wno-unused-value for clrjit projects May 15, 2026
Copy link
Copy Markdown
Contributor

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

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

@EgorBo
Copy link
Copy Markdown
Member Author

EgorBo commented May 16, 2026

PTAL @dotnet/jit-contrib minor cleanup, I'll do the clean up for unused vars separately as it emits too many changes (and probably will not remove that flag for variables)

No diffs

@EgorBo EgorBo requested a review from a team May 16, 2026 00:28
else if (isScalar)
{
reverseCond ? NI_X86Base_CompareScalarNotLessThanOrEqual : NI_X86Base_CompareScalarGreaterThan;
id = reverseCond ? NI_X86Base_CompareScalarNotLessThanOrEqual : NI_X86Base_CompareScalarGreaterThan;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

apparently, this was a bug

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 18, 2026 11:15

regNumber baseReg = id->idReg2();
if (baseReg != REG_SP || baseReg != REG_FP)
if ((baseReg != REG_SP) && (baseReg != REG_FP))
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this was a bug too (previous code was "always true") but given it just for insLatency (PerfScore) computation - it's not a big deal

Copy link
Copy Markdown
Contributor

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

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Comment thread src/coreclr/jit/CMakeLists.txt Outdated
Comment thread src/coreclr/jit/emitxarch.cpp
Comment thread src/coreclr/jit/lower.cpp
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 18, 2026 11:27
Copy link
Copy Markdown
Contributor

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

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

The Attributes.BoxedValues test now passes after dotnet#127596 added custom
attribute blob parsing and rewriting, but the entry was not removed
from ILTrimExpectedFailures.txt. CLR_Tools_Tests fails with:

  Test 'Attributes.BoxedValues' is in the expected failures list but
  now passes. Remove it from ILTrimExpectedFailures.txt.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@EgorBo
Copy link
Copy Markdown
Member Author

EgorBo commented May 18, 2026

ping @dotnet/jit-contrib simple clean up, no diffs

the idea is to enforce some warnings to prevent bugs like the one I fixed

Copilot AI review requested due to automatic review settings May 18, 2026 19:54
Copy link
Copy Markdown
Contributor

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

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

Comment thread src/coreclr/tools/ILTrim.Tests/ILTrimExpectedFailures.txt Outdated
Copilot AI review requested due to automatic review settings May 19, 2026 15:17
Copy link
Copy Markdown
Contributor

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

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

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants