This is an archive of the discontinued LLVM Phabricator instance.

[Support] Remove MSVC specific AlignedCharArray implementation
AbandonedPublic

Authored by RKSimon on Jul 9 2019, 6:47 AM.

Details

Summary

Now that D64326 has landed we can remove the old MSVC AlignedCharArray implementations as VS2017+ can correctly handles alignas with by-value arguments.

This patch raises the question of whether we want to use llvm::AlignedCharArray directly in the codebase or whether some/all cases should be replaced with a type that uses alignas() directly?

Diff Detail

Repository
rL LLVM

Event Timeline

RKSimon created this revision.Jul 9 2019, 6:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 9 2019, 6:47 AM
Herald added a subscriber: kristina. · View Herald Transcript
rnk added a comment.Jul 9 2019, 11:40 AM

I think the first version of MSVC 2017 doesn't allow you to pass aligned parameters when targeting x86, which has always been the problem child architecture:
https://gcc.godbolt.org/z/n_l_g5
Godbolt doesn't seem to have MSVC 2017 versions 19.11-19.13 to allow us to test.

LLVM claims it's minimum supported version is 19.11:
https://github.com/llvm/llvm-project/blob/456fc4fa6dc573c717f8db0d44a078f28060826e/llvm/cmake/modules/CheckCompilerVersion.cmake#L16

I think we need to check if 19.11 accepts this new code before we land this change, or we need to raise it the minimum to 19.14.

This patch raises the question of whether we want to use llvm::AlignedCharArray directly in the codebase or whether some/all cases should be replaced with a type that uses alignas() directly?

I think you're right, a lot of code can be simplified to use LLVM_ALIGNAS. I think we still need the LLVM_ALIGNAS portability macro until we raise our minimum GCC version, but it works more or less just as well as alignas.

RKSimon abandoned this revision.Aug 20 2019, 9:37 AM

This was handled in rL367282