FlatSymbolRefAttr should contain a StringAttr internally?

[lattner]

Bugzilla Link	51665
Resolution	FIXED
Resolved on	Aug 30, 2021 18:29
Version	unspecified
OS	All
CC	@jpienaar,@River707

Extended Description

Right now, symbols are defined with StringAttrs and referenced with FlatSymbolRefAttr. Both copy the underlying string data, which is bad for memory use (two copies of the same string) but also force clients to do string comparisons or rebuild the StringAttr when searching for the symbol instead of doing pointer comparisons.

I think it would be a straight-forward for FlatSymbolRefAttr to just hold the name as a StringAttr. We could keep exactly the same API, and add a "get the attr" accessor as well, which could accelerate this stuff. Is there any concern with that?

xref the StringMap in this patch, which I would like to be a DenseMap: https://github.com/llvm/circt/commit/8348c2144cef6724e5d7da6b5f75963334b4431b

-Chris

[lattner]

done thanks!

[lattner]

The first 2/3 of getSymbolRefAttr is moved in this patch: https://reviews.llvm.org/D108922

[lattner]

This is done in 41d4aa7de68e. I want to look at removing the Builder::getSymbolRefAttr API's though, and I'll keep this bug open to track that. I should get to it in the next couple days.

[lattner]

Patch up for review here: https://reviews.llvm.org/D108899

[joker-eph]

I could also imagine to decouple string internalization in the context from StringAttr, so that any StringRefParameter but that does not seem straightforward to me right now, so what you want to do LGTM.

[lattner]

btw, I'm happy to make the change if y'all think it is a fine thing to do.

[lattner]

assigned to @joker-eph