llvm / llvm/llvm-project

[GISel] Register GISelValueTracking as observer in Combiner/Legalizer to prevent future caching bugs

Open
#210,002 2 comments 0 reactions 0 assignees View on GitHub
backend:AArch64 llvm:globalisel
Dominant language
LLVM
Stars
40.5k
Forks
18.7k
PR merge metrics
PR metrics pending

Description

I was looking through the GlobalISel code and noticed that `GISelValueTracking` inherits from `GISelChangeObserver`, but we aren't adding it to the observer list in the `Combiner`, `Legalizer`, and other places before modifying instructions.

Since `GISelValueTracking` doesn't cache results right now (even identical calls to `computeKnownBitsImpl` are fully recomputed), this isn't causing bugs today. However, if we ever add caching to `GISelValueTracking` later on, not having it registered as an observer will definitely cause stale-cache bugs.

Would it make sense to add it to the observers now to prevent future issues?

Contributor guide

Open the contributing guide

Research direction

Start by tracing observer registration and instruction-mutation paths in the GlobalISel Combiner and Legalizer, then inspect GISelValueTracking and computeKnownBitsImpl. Done means GISelValueTracking is registered wherever required before modifications, with the existing observer behavior preserved.

Written by the indexing model from the issue text.

Assessment

Domain
compilers
Issue type
Refactor
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.