Page MenuHomePhabricator

[Subtarget] Remove static global constructor call from the tablegened subtarget feature tables

Authored by craig.topper on Feb 21 2019, 10:50 AM.



Subtarget features are stored in a std::bitset that has been subclassed. There is a special constructor to allow the tablegen files to provide a list of bits to initialize the std::bitset to. This constructor isn't constexpr and std::bitset doesn't support many constexpr operations either. This results in a static global constructor being used to initialize the feature bitsets in these files at startup.

To fix this I've introduced a new FeatureBitArray class that holds three 64-bit values representing the initial bit values and taught tablegen to emit hex constants for them based on the feature enum values. This makes the tablegen files less readable than they were before. I can add the list of features back as a comment if we think that's important.

I've added a method to convert from this class into the std::bitset subclass we had before. I considered making the new FeatureBitArray class just implement the std::bitset interface we need instead, but thought I'd see how others felts about that first.

I've simplified the interfaces to SetImpliedBits and ClearImpliedBits a little minimize the number of times we need to convert to the bitset.

This removes about 27K from my local release+asserts build of llc.

I'm open to any suggestions on better ways to do this.

Diff Detail


Event Timeline

craig.topper created this revision.Feb 21 2019, 10:50 AM
dblaikie accepted this revision.Feb 28 2019, 3:28 PM
dblaikie added inline comments.
179 ↗(On Diff #187820)

Perhaps either use Mask.length() as the loop bound (or a range-based for loop) and/or change the parameter to be a reference to an array of MAX_SUBTARGET_WORDS length? (rather than having an implicit contract where you must pass this function an array of that length, but nothing checks that)] & would just walk off the end of the array otherwise)

273–277 ↗(On Diff #187820)

This code looks like the same code over ImpliesMask above? (except for ImpliesMask rather than FeatureList) - perhaps it could be folded into the printFeatureMask function?

This revision is now accepted and ready to land.Feb 28 2019, 3:28 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 28 2019, 6:19 PM
bjope added a subscriber: bjope.Mar 4 2019, 2:27 AM
bjope added inline comments.

@craig.topper, I see lots of warning when compiling using clang 3.6 after this patch:

lib/Target/AArch64/ error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces]
  { "a35", "Cortex-A35 ARM processors", AArch64::ProcA35, { { 0x20800080800800ULL, 0x0ULL, 0x0ULL, } } },
                                                               {                                  }

Adding one more level of braces "{ { { 0x20800080800800ULL, 0x0ULL, 0x0ULL, } } }" seems to work, but would that mean that we no longer run the new constexpr ctor?

bjope added inline comments.Mar 4 2019, 10:49 AM

Ok, now I've read this:

So as described in the example

std::array<int, 3> a1{ {1, 2, 3} }; // double-braces required in C++11 prior to the CWG 1270 revision
                                    // (not needed in C++11 after the revision and in C++14 and beyond)

I guess my old clang 3.6 compiler doesn't include CWG 1270 then.

Afaict clang 3.6 should still be supported (even if I don't know how many warnings that are accepted when building).
Should we simply generate the double braces here, to work around these warnings?

@bjope , thanks! I'll add another set of curly braces.