@chandlerc I've reverted the patch r350402 sorry for causing the performance issue. I'll investigate the datalayout idea a bit more, @miyuki original solution to the function pointer aligment issue did something like that https://reviews.llvm.org/D44781

This was approved previously https://reviews.llvm.org/D55115 but when committed it caused failures on the sanitizer buildbots when building
llvm with clang (containing this patch). This is now fixed because I've added a check to see if getting the parent module returns null if it does then set the alignment to 0.

Updated to use getPointerAlignment and removed test file that was reliant on compiler using assumptions about function alignment.

Looks fine, I think it's ready to land. Do you have commit access ? If not do you want me to commit on your behalf ?

Thanks for reviewing James.

ok thanks.

Thanks for reviewing. Before I commit could you tell me if I need to update any build systems e.g. buildbots ?

This works for me, but the idiomatic macro usage in libc++, and (hopefully) libc++abi should always use !defined(FOO) as opposed to !FOO. I'll clean this up in the next week if nobody else wants to.

Going for post-commit.

Ah sorry for the mistake. Fix is here https://reviews.llvm.org/D30343

It's fine as a pass. I mean, it would be nice to include as part of some existing pass, but I'm not sure where it would fit in.

Thanks for your review comments Eli.

Thanks for the review Tim, I really appreciate it.

Ping.

Hi Amjad,

I'm surprised you didn't pick this problem before commit. Our buildbots seem happy, so this was definitely a downstream test.

Hi Renato,

Thanks for reviewing Renato, will make your suggested changes before committing. Can you also review the clang part of the patch at some point http://reviews.llvm.org/D21179 ?

Thanks for the review Renato. I replied back to your comments below.

LLVM part of the patch is here http://reviews.llvm.org/D21178

Tim thanks for reviewing this patch. I've uploaded a new one with your suggested change. If I don't respond to any further comments after today it'll be because I'm on holiday and won't be back till next week.

LLVM part of the patch is here http://reviews.llvm.org/D20564

http://reviews.llvm.org/D20394 which adds a test for the intrinsic in llvm

Hi Renato,

It's been our stance for a long time to require docs to approve changes, however small. I don't want to relax that which I think is a good constraint, not for such a seemly irrelevant issue.
I also doubt this will be the only addition in the new ACLE, so why not release the document, and then submit all changes then?
Or, maybe I am mistaken, and this is really that important... is it?

Hi Michael,

Hi Michael,

Thanks for the review.

In D10542#192727, @mkuper wrote:

Hm. I've read the patch more carefully, and I'm actually somewhat confused now.

Each target has two enumerations:

1. SubtargetFeatureFlag - this is used, I think, only by the AsmParser, and has elements that look like Feature_HasAVX512.

This one still lives in a regular bitfield, not a bitset, so the elements of the enum are of them form "1 << C".
If I'm not mistaken, ComputeAvailableFeatures() returns a set of these values.

2. A nameless enum used everywhere else, which has elements that look like X86::FeatureAVX512.

I've changed this enum in my original patch, and now these are ordinal values (1, 2, 3... etc.), instead of powers of 2.

Now, it looks like what you did was to change everything that used bitfields containing SubtargetFeatureFlag to use a FeatureBitset instead. But the values themselves are still powers of 2, so you are still bound by the limits of the underlying type.

Is that right? If it is, then what is the goal of this patch?

Hi Michael,

Hi Javed,

Why are you testing for _ARM_FP if the core of what you're doing is adding +/- strings to -target-features? Why not test the target-features directly?

I don't think this will ensure the old code fails.

can someone have another look at this?

Thanks for the review Michael.

Thanks for the review.

Can you explain in more detail how I can check for this internal bug using CHECK-DAG?

I'm assuming this is the target features, that normally come in the format:

+foo,-bar,+baz

or

-target-feature +foo -target-feature -bar -target-feature +baz

Either case, if you use:

CHECK-DAG: +foo
CHECK-DAG: -bar
CHECK-DAG: +baz

They could come in any order and it would still match.

The order that the features are specified on the cc1 command line won't mean that the features will be stored in the Features vector in the same order. This is because the features on the command line are parsed and stored into a StringMap then they're taken out again which can jumble up the order because StringMap's don't maintain order, below is the code which takes the features out of the StringMap in lib/Basic/Targets.cpp:

Changed 'HW_FP_remove =...' to 'HW_FP_remove |=...'

TargetInfo::CreateTargetInfo takes the features from the command line and puts them into a StringMap and pulls them out again which can jumble the order of the features. So if I did write a test there's no guarantee that handleTargetFeatures will see them in the same order that I specified them on the command line. If you have any suggestions on how I can write a test for this then please let me know.

Hi James,

LGTM, but someone else should approve as well

LGTM, but someone else should approve as well