Page MenuHomePhabricator

Replace the bitfield of SubtargetFeatures with a BitVector
ClosedPublic

Authored by mkuper on Jan 20 2015, 1:01 AM.

Details

Summary

SubtargetFeatures uses a uint64_t bitfield to store the SubtargetFeatures of each target.
Unfortunately, at least for x86, we've run out of bits.

Per Chandler's suggestion, I tried to convert this into a BitVector, but I'm not sure it makes sense, especially from a compile-time standpoint.
One potential problem is the look-up times.
The other is that the existing uint64_t is being passed by value into all sorts of places. One issue with replacing it with a BitVector is minimizing the number of copies. The other is that some odd things are done with it, like shoving it into a union. I replaced that particular case with a pointer to a BitVector (which should stay alive for the duration), but I'm not sure how safe that is.

Does this look like something we can do? If not, any other suggestions?

Note that the patch itself needs quite a lot of cleanup, among other things because of stuff missing in the BitVector interface (e.g. everything that uses .flip() right now). If there is consensus that this is sane, I'll clean it up and post a new version for review. There's also the bit with having to pass the size of the vector as part of the initializer list, which I would rather avoid, but I don't really see how.

(The patch is fairly large, but a lot of is trivial changes - replacing "getFeatureBits() & X" with "getFeaturesBits()[X]")

Diff Detail

Repository
rL LLVM

Event Timeline

mkuper updated this revision to Diff 18419.Jan 20 2015, 1:01 AM
mkuper retitled this revision from to Replace the bitfield of SubtargetFeatures with a BitVector.
mkuper updated this object.
mkuper edited the test plan for this revision. (Show Details)
mkuper added a reviewer: chandlerc.
mkuper added a subscriber: Unknown Object (MLST).
chandlerc edited edge metadata.Jan 20 2015, 1:14 AM

FWIW, I don think this is generally the right direction.

Some simplifying thoughts:

  1. Look at fixing the incompatible usages prior to changing the type. For example, putting the bits into a union seems like a bad idea anyways.
  1. Is the number of bits a compile time thing? If so, then std::bitset would work much better than BitVector and likely avoid some of the complexity here. There is also SmallBitVector if the number is dynamic but usually small.
mkuper updated this revision to Diff 18438.Jan 20 2015, 8:48 AM
mkuper edited edge metadata.

Second try, this time with a bitset (or, rather, a wrapper around bitset to enable initializer lists).

Also, can anyone on the AArch64 side help with the unions, perhaps?

Ping.

Tim, do you want me to make this change to the AArch64 part as is?
You said you may have another idea.

Regardless, could anyone take a look at the wrapper around bitset, to make sure this makes sense?

t.p.northover edited edge metadata.Jan 27 2015, 8:16 AM

Tim, do you want me to make this change to the AArch64 part as is?
You said you may have another idea.

I thought I'd already applied the AArch64 change (there's no more
Features in the union, so probably no refactoring needed for you). But
you'll probably have quite a few merge conflicts to solve before
committing.

Cheers.

Tim.

Argh, sorry, missed that.

mkuper updated this revision to Diff 18820.Jan 27 2015, 9:26 AM
mkuper edited edge metadata.

Updated the AArch64 part following Tim's change.

Hi,

What is the current status of this patch? The ARM backend is also getting close to the 64 SubtargetFeature limit, so it would be good to get this committed soon.

Thanks,
Oliver

It'd probably be good to get Chandler's views too, but the patch seems OK to me.

Tim.

Talked to Chandler, he is rather busy and asked me to find some target people to review it if I can. :-)

Can someone verify that this look sane?
Especially the changes to TableGen, and the way FeatureBitset itself is defined.
Most (although not all) of the changes to the targets themselves are fairly mechanical.

Can someone verify that this look sane?

I think it does.

Cheers.

Tim.

Hi,
So, it is a chance this will be committed soon?

Thanks,
Vladimir

Looks like there are no objections, so, yes.

In fact, rebasing this to ToT at this very moment (which is a bit of a hassle given the extent of the changes).
Will commit after the rebase, assuming all is well.

This revision was automatically updated to reflect the committed changes.
vsukharev added a comment.EditedFeb 19 2015, 6:46 AM

Hmm, it seems the patch should have also changed the following file accordingly
include/llvm/MC/MCTargetAsmParser.h: uint64_t getAvailableFeatures() const { return AvailableFeatures; }

And also all related stuff. Otherwise, it will be impossible to use these features in AsmParser to initialize Mappers properly
Or.. is it inventionally left as uint64_t, as I could see in utils/TableGen/AsmMatcherEmitter.cpp :

- OS << " uint64_t ComputeAvailableFeatures(uint64_t FeatureBits) const;\n";
+ OS << " uint64_t ComputeAvailableFeatures(const FeatureBitset& FB) const;\n";

If so, what's recommended way to restore FeatureBitset from uint64_t MCTargetAsmParser::AvailableFeatures ?

These are two distinct bitfields - one for subtarget features, and one for "available features", which are a subset of the target features that participate in instruction matching in AsmParser. As far as I know, no platform is close to the 64-bit limit in terms of available features right now.
There is no requirement for these two to be in sync. In fact, for a very long time, they weren't. SubtargetFeatures has been upgraded from 32-bit to 64-bit at r129590 (April 2011). AvailableFeatures was only upgraded to 64-bit in r215887 (August 2014).
The translation from SubtargetFeatures to AvailableFeatures is performed by ComputeAvailableFeatures(). To the best of my understanding, there has never been a way to recover the subtarget features back from the available features. This makes sense to me, since the transformation is lossy.

In any case, once this actually works (see separate email about revert due to ARM/MIPS/PPC issues), we can also think about an equivalent patch for available features, but I'm not sure that's necessary in the short term.

vsukharev added a comment.EditedFeb 19 2015, 10:30 AM

Oops, this way it's currently impossible to refactor AArch64 backend in this manner. r207742 introduced bug - misusing these two types.
Currently, both types are used for Mappers initialization, that's wrong.

I am working on the fix now. After it, your refactoring will be possible.

Hi,
my investigations finally showed AArch64 backend doesn't have a bug.
At it's been told, it's a huge mess instead. The following field and methods mean completely different things in classes MCTargetAsmParser and MCInstPrinter, despite the spelling are identical.

uint64_t AvailableFeatures; and
uint64_t getAvailableFeatures(); void setAvailableFeatures(uint64_t Value);

We should just take care, which one to use. It will be simpler after the refactoring proposed here.
Do you have any success continuing this so much needed work in other backends after revert?

Hi Michael,

I'm about to test this on my box, but the patch doesn't apply cleanly. Can you please rebase it?

thanks,
--renato

I couldn't figure out how to update an already-closed revision.
Uploaded a rebased version as http://reviews.llvm.org/D8542

Renato, can you check if you can self-host with this on ARM/AArch? If you can, I'll try to commit it again.
(Or I could just commit, and see what the fallout is :-) )