[llvm] [SimplifyCFG] When only one case value is missing, replace default with that case (PR #76669)

Quentin Dian via llvm-commits llvm-commits at lists.llvm.org
Tue Jan 2 05:03:17 PST 2024


DianQK wrote:

> What's the purpose of adding the profile-data to the test case that's been updated, does it not work without that?

This is just to verify that we are updating the profile-data correctly. I've added a test case without profile-data.

> The original author apparently wished there to be a negative test case, if it's possible to keep a negative case and just add another test function for the new behaviour, IMHO that's preferred. (Perhaps that's not possible, but it would mean that the test coverage strictly improves).

As I was adding test cases, I found this one that needs to be updated. Retrieved from 98a2dabc08fd89375f3af4158eb55575be58f13c, cc @preames.

> The change here looks good -- however all the rationale for why this change is being made is in the rust bug report you link to. Could you please elaborate the comment on the added code ("When only one case value is missing...") to explain why this is a good thing to do. There's a risk that someone in the future tries to delete the code if it's not clear to them why it's beneficial.

I have added detailed explanations in PR.

> To confirm my own understanding: adding an unreachable default is fine because we know the bits being switched upon are in a small finite set of cases; the new form has a better structure / is easier to codegen for? If so, sounds like a good change, LGTM.

Yes, we no longer need to think about the default branch.

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


More information about the llvm-commits mailing list