llvm / llvm/circt

[FIRRTL] Dedup should properly handle DistinctAttr, including its use in paths.

Open
#6,583 0 comments 0 reactions 0 assignees View on GitHub
enhancement FIRRTL
Dominant language
C++
Stars
2.2k
Forks
524
Avg merge
3d 2h
Merged PRs (30d)
46

Description

In https://github.com/llvm/circt/pull/6582, initial support for classes and objects was added in Dedup.

However, the way we use DistinctAttr with paths is not compatible with Dedup, almost by definition. Two paths, which could dedup if the entities they point to dedup, will have different DistinctAttrs. The initial PR does not handle this, and simply ignores DistinctAttr. We have verifiers and logic in later passes that ensure any paths that came out of dedup are at least valid, but this is not sufficient: Dedup should check, and only dedup two paths if the entities they point to dedup.

I have some thoughts about how to approach this, which I will share here, but for now I want to file this ticket to at least explain the "what", if not the "how".

Contributor guide

No contributing guide indexed for this repository

Research direction

Start by reading PR 6582 and the Dedup implementation it introduced, then trace how DistinctAttr is used on paths and how the later verifiers validate them. Done means Dedup checks the entities referenced by paths and only deduplicates paths when those entities can also be deduplicated, rather than ignoring DistinctAttr.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
compilers
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.