This is an archive of the discontinued LLVM Phabricator instance.

[TargetParser][AArch64] Reorder enum to preserve 5.0.0 libLLVM ABI.
ClosedPublic

Authored by mcrosier on Nov 2 2017, 8:54 AM.

Details

Summary

This is required for backporting r311659 to the 5.0.1 release, which is a prerequisite for backporting support for Qualcomm's Saphira CPU to the 5.0.1 release.
PR35060

Diff Detail

Repository
rL LLVM

Event Timeline

mcrosier created this revision.Nov 2 2017, 8:54 AM
rengolin edited edge metadata.Nov 2 2017, 8:58 AM

Why did they change in the first place?

fhahn added a subscriber: fhahn.Nov 2 2017, 8:58 AM
rengolin accepted this revision.Nov 2 2017, 9:08 AM

Ha, I see. That was your commit trying to reorder things. :) LGTM.

This revision is now accepted and ready to land.Nov 2 2017, 9:08 AM

Would it make sense to add a test, so that any future changes doesn't undo this behaviour?

Would it make sense to add a test, so that any future changes doesn't undo this behaviour?

rL311659 (which reordered the enum in the first place) already has test coverage, however, I don't think that's the kind of testing you're suggesting.

I think what you're suggesting is that we add unit tests to ensure the enums don't get reordered. AFAIK, we don't have a hard/fast rule for this (I could easily be mistaken). IIUC the issue only arises when you want to backport a change so that, for example, the libLLVM.so shipped with the 5.0.0 release is compatible with the 5.0.1 release. I don't know if we ensure major releases are compatible with this respect to one another.

Do you have any thoughts on this, Renato?

Why did they change in the first place?

The reordering was arbitrary, but I didn't realize the implications at the time. :/

Why did they change in the first place?

The reordering was arbitrary, but I didn't realize the implications at the time. :/

Been there, done that. :)

I don't think we need tests for testing the ordering. The original patch already had tests, we should be fine.

Also, this would be good to be on trunk as well as 5.0. We can't break binary compatibility between dot-releases, but we shouldn't do so on normal releases either, unless there's a reason.

--renato

Why did they change in the first place?

The reordering was arbitrary, but I didn't realize the implications at the time. :/

Been there, done that. :)

I don't think we need tests for testing the ordering. The original patch already had tests, we should be fine.

Also, this would be good to be on trunk as well as 5.0. We can't break binary compatibility between dot-releases, but we shouldn't do so on normal releases either, unless there's a reason.

I agree.

--renato

This revision was automatically updated to reflect the committed changes.