Cherry-pick 345826, 345839, and 345838 into 7.0.1

[erichkeane]

Bugzilla Link	39540
Resolution	FIXED
Resolved on	Dec 07, 2018 21:08
Version	7.0
OS	Windows NT
Blocks	#38454
CC	@zygoloid,@tstellar
Fixed by commit(s)	r342152 r345826 r345838 r345839 r348681 r348682 r348684 r348686

Extended Description

All 3 are tough-to-find but significant bugs with CPU-Dispatch and function multiversioning issues. They have no impact other than for MV functions.

[erichkeane]

assigned to @zygoloid

[tstellar]

Could you add a link to any post/pre commit reviews for these packages. I'm trying to figure out who to ask to look at these.

[erichkeane]

https://reviews.llvm.org/rL345826
https://reviews.llvm.org/rL345839
https://reviews.llvm.org/rL345838

There weren't really any review comments on these, but both are all fixups of the cpu-dispatch functionality that I implemented a while back.

[tstellar]

Richard, what do you think about these?

[zygoloid]

r345826 looks OK for branch (I'm not sure it's really the right thing in the long term, as it looks like it will call cpu_specific(pentium) code when running on a pre-pentium chip, but at least fixing the assertion in this case seems like a good change for the branch).

r345839 looks wrong to me -- we should only emit MV function variants when they're referenced (for instance, by the resolver). Clang trunk emits inline multiversion functions even when they're not referenced, which is definitely a bug, and will significantly bloat IR when including a large header of multiversion inline functions -- though my initial testing shows that there was some other reason we had that bug prior to this change, so it might not be a regression caused directly by this change. To the extent that this change is necessary, it seems to be papering over a bug elsewhere.

If possible I'd like to get the underlying bug here fixed properly for 7.0.1 (though time is clearly short). Failing that, this change is an acceptable but unfortunate workaround.

r345838 looks OK for branch.

[erichkeane]

What is our intended schedule for 7.0.1?

I can make the r345839 my highest priority starting Monday, so if I've got a touch more than a week, I should be able to find an alternate fix and get it up for review.

[tstellar]

What is our intended schedule for 7.0.1?

We are past the deadline for new merges, and I don't want to delay it any more, so we don't have time to develop an alternative fix and get it into 7.0.1.

[erichkeane]

Regarding r345839 :

I've reverted it in r348595. After discussions with Richard, I realized the problem is much more limited than I initially thought. It applies only to inline functions marked with cpu_specific.

While the fix has no impact outside of multiversioning, I no longer think it is as significant as I originally thought. If we choose not to cherry-pick it I don't think the bug would be too significant.

I've committed an actual fix that only solves the problem (not patching it over as Richard said) in r348600.

Also note there is a non-consequential test fix in 348598 having to do with dispatch, but it is a fix-test only.

[tstellar]

I don't think we should cherry-pick r345839 if it was reverted in trunk.

That leaves us with merging r345826 r345838.

Richard any thoughts on the 'proper fix' merged in https://reviews.llvm.org/rL348600. Is this something we want to try to merge or should we leave it out? I want to do the final release candidate today.

[tstellar]

Erich, Richard, and I discussed this on IRC and agreed to merge the original 3 commits plus a refactoring commit: r342152

[tstellar]

Merged: r348681 r348682 r348684 r348686