Casting a zero should always be permitted. Make sure the canCoerce check reflects that, and that all users explicitly handle it.
I really like what you're trying to do here, but the actual change is problematic. You're mixing non-functional refactoring with semantic change which makes this really hard to understand. Please split this into at least two parts. (I say at least because you appear to have some code motion changes which could be landed trivially without review as well.)
Thanks @reames, I appreciate what you've already done here. I had started this a long time ago, where more of it was a functional change, then discovered you had recently already made most of the essential fixes and written the necessary tests. I've changed this into a stack of 3 commits that touch slightly different areas.
LGTM specifically to the noted parts only. Remainder to be rebased on landed parts and reviewed after a week or so of burn confirms belief named parts are NFC.
|49 ↗||(On Diff #196653)|
Consistency: we should probably be avoiding mmx here based on constant folding logic and langref, but given the original code being restructured doesn't handle this, I'm okay with this being done as followup.
|52 ↗||(On Diff #196653)|
I'm okay with the above split landing as a NFCI, then reviewing the rest.
|330 ↗||(On Diff #196653)|
This are grouped with the split above.
Comment change LGTM if you want to land this separately. (Please do.)
This looks on the surface to be a bug that needs fixed in the caller. Maybe there's a subtly I'm missing here though?
Given we know this *can't* be a scalable type, the generalization to type size doesn't seem to add any value here?
It's not really clear to me what value you get from splitting out the type based vs value based variants. You duplicate code and there doesn't seem to be any benefit (in this patch). I am tempted to reject this change.
This looks odd. How can this be NFC if you're actually changing the generated code?
I suspect that your patch description is stale and needs updated. If so, you're also missing tests.