[PATCH] D120714: [CodeGen] Place SDNode debug ID declaration under appropriate #if

Wed Apr 6 06:21:57 PDT 2022

kovdan01 added a comment.

In D120714#3432615 <https://reviews.llvm.org/D120714#3432615>, @thakis wrote:

> +1 to craig.topper's point. It's true that this is what ABI_BREAKING_CHECKS is for, but we should only use this if it saves significant memory. This uint16_t lives next to two bools, so removing it doesn't save any memory (it'll just become padding) as far as I can tell.
>
> Have you seen an actual memory use improvement from this?
>
> As far as I can tell, this adds a bunch of complexity without a benefit (?)

Hmm, I missed the fact that removing PersistentId will not save memory because of alignment and padding. Initially the motivation was more about stylish improvement – not having fields that we do not need, and then I just extrapolated that to possible memory usage enhancement without keeping padding in mind.

Experimented with that stuff, and I actually do not see memory usage decrease, you are correct: sizeof(SDNode) remains 80 on x86-64 linux. Regarding SelectionDAG – its layout, I suppose, could be changed somehow to save memory, but that will not give noticeable improvements because we do not have many SelectionDAG objects.

I suppose that the patch might become useful in terms of memory usage enhancement in future (if SDNode’s layout changes somehow), but as for now that’s more about codestyle.

If your usecase requires having PersistentId declared and kept correct in all build types and/or complexity looks inapplicable for you, I can revert the patch and submit a new one adding a commect why we do not place these fields under defines not to confure anyone more.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120714/new/

https://reviews.llvm.org/D120714