[llvm] [SimplifyCFG] Use hash map to continue hoisting the common instructions (PR #78615)

via llvm-commits llvm-commits at lists.llvm.org
Tue Mar 26 10:29:01 PDT 2024


amehsan wrote:

> This PR is really pushing the bounds of what is appropriate to do in SimplifyCFG. The code has become quite hard to understand.
> 
> My primary concern with this implementation would be with non-determinism -- I don't think that your approach of only supporting hoisting for the first instruction with a given hash is viable (unless I am misunderstanding something).

Hi @nikic

Your comment here is a bit ambiguous and it is appreciated if you could clarify. From the first sentence it looks like you have a fundamental objection to the design of the solution. But the rest of comment highlights two specific issues:

1. Issue of non-determinism on that you are completely right and Rouzbeh is going to fix it.
2. Code being hard to understand. This is a vague statement. I am sure you agree that there are many pieces of codes in LLVM that are very complex for good reasons. But I agree that code should not be more complex than needed. I think it is better to highlight where/how the code is unneccessarily complex. In and of itself complexity of the code is not a valid objection.

I really appreciate your time and effort to help review this PR.





https://github.com/llvm/llvm-project/pull/78615


More information about the llvm-commits mailing list