Conversation
AndyAyersMS
commented
Feb 26, 2026
- Fix quite a few typos in comments
- Fix a retyping issue in inlining
- Fix argument check logic for inline observations
- Remove some unreachable, unused, or redundant code
- Fix dangling BB0 in Wasm
- Fix some cases of shadowing
- Fix copy-paste bug: isArg1Arg was computed from arg0 instead of arg1 in SRCS_UNSAFE binary intrinsic handling (line 1315) - Fix unreachable code: duplicate BBJ_EHFILTERRET check should be BBJ_EHFINALLYRET, enabling endfinally validation (line 4169) - Fix form-feed escape \f in JITDUMP format string (line 76) - Fix wrong #endif comment: TARGET_XARCH should be TARGET_ARM64 (line 1422) - Fix typo 'regulary' -> 'regular' in two comments (lines 1340, 1398) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add assert(pred == edge) in fgRemoveRefPred to verify the edge found by linear search matches the edge being removed. - Add early return in fgRedirectEdge when the destination is already the target, avoiding unnecessary pred list manipulation and a false fgModified signal. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Fix operator precedence bug: *metric++ increments the pointer instead of the value. Changed to (*metric)++ so the metric counter is actually bumped when profile data becomes inconsistent. - Fix wrong InstrumentationKind check in BlockCountInstrumentor::Instrument DEBUG path: EdgeIntCount can never match for block counts; changed to BasicBlockIntCount so 32-bit counters are written correctly. - Fix swapped arguments to fgGetPredForBlock in edge instrumentor DEBUG path: was searching source's pred list for target (reverse edge) instead of target's pred list for source (correct edge). - Remove dead code: missingEdges variable was initialized to 0 and never incremented, making the check always false. - Fix typos: unsychronized, Don't instrumenting, affeted, bome. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Fix use-after-free: stack-local BasicBlock bb0 was stored in fgIndexToBlockMap and accessed during codegen after the function returned. Heap-allocate it from the JIT allocator instead. - Fix comparator strict weak ordering violation in comesBefore lambda used with jitstd::sort: when both intervals are loops with the same chain start and end, comesBefore(x, x) returned true, violating irreflexivity. Now returns false for equal elements. - Remove unused 'first' variables in DumpDot() and FindNested() DEBUG blocks. - Fix parameter name mismatch in WasmDfs declaration (ByEH -> ViaEH) to match definition and all call sites. - Fix typos: assingnment, putcode, lookin. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Fix ReverseLikelihoods: function was a complete no-op. The inner loop iterated over successors but never used the loop variable, the likelihoods vector was reversed in place but never written back to edges. Removed the spurious inner loop and added write-back logic. - Fix RandomizeLikelihoods: reserve(N) sets capacity but not size, making subsequent operator[] access undefined behavior. Changed to resize(N, 0) so elements are properly constructed. - Remove redundant SumOutgoingLikelihoods call in BlendLikelihoods: the outer call at line 672 computed sum for every block (including no-successor blocks) but the result was unused and shadowed by an inner declaration. Moved the call into the BBJ_COND/BBJ_SWITCH case. - Fix typos: multply, ony. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fix tree->OperIs(GT_IND) to inlineCandidate->OperIs(GT_IND) in LateDevirtualization - tree is always GT_RET_EXPR so the check was dead code. The intent is to check the inline candidate node for RVA static byref reinterpretation. Also fix stale comment header (AssignStructInlineeToVar -> StoreStructInlineeToVar) and typos: obtain -> obtained, indicies -> indices (3x), ensabled -> enabled, assetionProp -> assertionProp. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Clarify inThisFlt as alias for inThisHnd (filters use handler index; the distinction is made by acdKeyDsg). Rename shadowing variable 'map' to 'acdMap' in fgCloneTryRegion to avoid shadowing the outer BlockToBlockMap. Fix inverted comment about fgCloneFinally direction. Fix typos: an->and, tested trys->nested tries, lexcially->lexically, indicies->indices (2x), thge->the. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Fix fgDumpFlowGraph always returning false (result never set to true) - Remove duplicate BBF_HAS_NEWARR check that produced duplicate XML attribute - Remove unused variable phiArgs in fgDumpBlockMemorySsaIn - Remove dead debug code (if verbose && 0) - Fix 7 typos in comments Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Change bool addedTemps = 0 to bool addedTemps = false - Remove dead debug code (if verbose && 0) in fgCompactBlock - Remove unused variable firstStmt in fgBlockEndFavorsTailDuplication - Remove unused outer reason variable shadowed by inner declaration - Update stale comment about loopIterations in fgOptimizeBranch - Fix 4 typos in comments (trys, The are both, number of time) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
@EgorBo this was largely copilot reviewing existing code. It looked at all the fg* files. FYI @dotnet/jit-contrib The retying change in fginline leads to some GC info diffs in Bepu in the RWC collection. The rest of this appears to be no diff. |
There was a problem hiding this comment.
Pull request overview
This PR cleans up JIT flowgraph-related code by correcting typos, fixing a few logic/typing issues (notably in inlining and instrumentation/debug paths), and removing unreachable/redundant code. It also includes a WASM-specific fix to avoid publishing a dangling BB0 pointer.
Changes:
- Fix correctness issues in flowgraph/instrumentation helpers (e.g., edge lookup argument order, strict weak ordering in interval sort, inlining retyping check).
- Address WASM flowgraph lowering stability by heap-allocating the published “fake BB0”.
- Remove dead/unreachable debug code and correct numerous comment typos.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/jit/fgwasm.h | Renames out-param to clarify EH-only reachability meaning. |
| src/coreclr/jit/fgwasm.cpp | Fixes WASM BB0 lifetime (heap allocation) and comparator strict-weak-ordering. |
| src/coreclr/jit/fgprofilesynthesis.cpp | Refactors likelihood reversing/randomization; fixes a likelihoods container sizing bug. |
| src/coreclr/jit/fgprofile.cpp | Fixes instrumentation-kind check, corrects edge lookup argument order, and cleans up debug logic. |
| src/coreclr/jit/fgopt.cpp | Minor correctness/cleanup (bool init) and removes dead debug code/unused locals. |
| src/coreclr/jit/fginline.cpp | Fixes inlining retyping condition and various comment typos. |
| src/coreclr/jit/fgflow.cpp | Strengthens edge removal with an assert and avoids redundant redirects. |
| src/coreclr/jit/fgehopt.cpp | Corrects filter/handler tracking logic and improves naming/typos. |
| src/coreclr/jit/fgdiagnostic.cpp | Removes unused debug printing code and fixes comment typos; tweaks dump result handling. |
| src/coreclr/jit/fgbasic.cpp | Fixes a JITDUMP escape issue and corrects some control-flow validation logic/typos. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Diffs shows the Bepu Physics changes I noted above, but no other difss. Plus a TP improvement in minopts. I am not sure what would explain the TP improvement, other than maybe the short-circuit in |