[PATCH] D124878: [Bitcode] Include indirect users of BlockAddresses in bitcode

Duncan P. N. Exon Smith via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue May 10 09:55:48 PDT 2022


dexonsmith added a comment.

In D124878#3495317 <https://reviews.llvm.org/D124878#3495317>, @twd2 wrote:

> In D124878#3492498 <https://reviews.llvm.org/D124878#3492498>, @dexonsmith wrote:
>
>> At a high-level, the design for writing bitcode is a two-pass traversal algorithm:
>>
>> - First pass in ValueEnumerator indexes everything
>> - Second pass in BitcodeWriter writes things
>>
>> It feels like this new logic could be put into the first pass in ValueEnumerator. (Probably the previous patch, which this builds on, should have done that as well, to avoid iterating over `users()` in BitcodeWriter, but I guess I didn't notice!)
>
> I just tried your advice to update this patch (not uploaded yet) but found the existing logic is
>
>   for each function
>       incorporateFunction, which enumerates function-local constants, including BlockAddresses, and we can maintain the users of referred foreign functions here
>       writeFunction, which emits BLOCKADDR_USERS records and the function-local constants
>
> However, for every function, before emitting BLOCKADDR_USERS records, we should expect that we have visited all the constants, including function-local ones, and thus we have collected all the users of the blockaddresses in the function.
>
> Can we enumerate (and thus emit) all the constants at the beginning, or add a traversing loop dedicated to collecting the users of blockaddresses?
> The latter will touch all the constants one more time.

Ah, sorry for the misdirection; I'd forgotten that incorporate+write are in an inner loop. Enumerating all function-local constants ahead of time would have side effects on the bitcode output (effectively changing numbering to be global). Traversing all constants an extra time just to index blockaddress (without enumerating) would penalize serialization time in the common case, where there are no/few blockaddresses.

I suggest for now that you go ahead with "traverse `users()`" logic for fixing the bug (maybe it'd be better to move the logic to ValueEnumerator, but I'll leave that up to you).

(A more invasive change would be to index in ValueEnumerator as I suggested, and write a placeholder record in functions that export BlockAddress that can be patched up later somehow after indexing is complete (the patch could point into a new bitcode block). This would probably add more complexity than it'd be worth though...)


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

https://reviews.llvm.org/D124878



More information about the llvm-commits mailing list