Thanks for fixing this !

@dexonsmith - Are you OK with that ?

Could I please get a review on this ? Thanks :) !

@dexonsmith @erik.pilkington The change is final now, could we get this in ?

Updating tests.

@dexonsmith what regression are you referring to ?
What this change is effectively doing now is changing the numbering of the blocks from incremental to hash-based.
So the demangler functionality remains the same (i think) - I saw that it is ignoring the (currently incrememntal) number, so changing it shouldn't make a difference.
Did I miss anything ?

@erik.pilkington would the hash-based numbering be OK for now ?

@erik.pilkington How about just adding numeric hash of block parameters for now ? This way we don't have to change the mangling / demangling scheme at all.
The mangling / demangling changes bring me into parts of LLVM that I'm not familiar with.
(PS - I still have to update tests).

For tests - the existing block tests should be enough, just need to update them.
I updated a few - want to make sure changes are final before updating all the tests to not have to update tests again.

@dexonsmith, @erik.pilkington - how about this ?

@dexonsmith - I think that should work - like this ?

@erik.pilkington / @vsk / @dexonsmith - how block name = hash(parent_function_name + block_type) ?
So any blocks that are inside the same parent function + have the same type will get an incremental number to de-duplicate names.

@rjmccall I see, I'm not that familiar with non-LTO workflow, I'll look into adding what needs to be added to have this also work for non-LTO. I was imagining It would be lot more integration.
Any other thoughts on this ?

@rjmccall - Yes, by "extra logic" I was referring to having to add integration with the non-LTO scenario. All the fundamental "logic" for computing offsets should already be here.
The pass is LTO-specific because this is the only scenario I'm trying to address right now - no other reason. Are there any cons to leaving it as a TODO for the future ?

@rjmccall - It can be implemented, sure but it would require extra logic that is not included here.
How common is this scenario ? I was targeting LTO only for now. Would it be OK to leave this as a TODO ?

@rjmccall - It does not. This would only work if all interface implementations were inside the same translation unit. But since that is not a common scenario it was not implemented to support this.

@dexonsmith - any suggestions to move forward on this ?

@vsk / @dexonsmith - I've added some more info in latest comments. Let me know if I can provide more info / context on this to be able to reach a conclusion. Or if you think it is clear at this point that the hash-based approach is a no-go.

@dexonsmith I did a benchmark with the worst case example I can come up with - 1000 regular functions and 1000 blocks : https://paste.ofcode.org/CFU6b46nuAA6ymxUEpkzka
I didn't manage to measure any performance decrease (the performance decrease was within the noise of repeated runs) - so seems the hashing is insignificant compile-time-wise even in worst case scenario (where most of the code is in function blocks).
And here is the actual text that is being hashed - the textual representation of the AST - this is not llvm IR with metadata: https://paste.ofcode.org/bfMzhbvHz9HRT7mVMe48Mx

@vsk - about breaking existing workflows - I was referring only to if / when this gets shipped out as the default - all the names for the function blocks will change and this might cause issue with tooling that relies on symbol names being consistent across builds.

@vsk - sure will add tests when removing from RFC.
As for making it default - would rather have this under a flag as hashing the block contents does have some overhead and I imagine this feature wouldn't be beneficial in most scenarios. Also, unexpectedly (by default) changing the name of the function blocks might have a negative impact on some existing workflows.

I don't have commit access. steven_wu can you please commit this for me ?

Indeed it can, that is another fix. I was just doing the "obvious" fix since I assume there is a reason why the ::codegen(Module &TheModule) function exists.
Should we just remove the function altogether ? As-is it is not thread-safe = results in crashes.
Also, a clone of TMBuilder is not expensive considering the context.