Page MenuHomePhabricator

[TableGen] Fix excessive compile time issue in FixedLenDecoderEmitter
ClosedPublic

Authored by foad on Mar 5 2021, 7:37 AM.

Details

Summary

This patch reduces the time taken for clang to compile the generated
disassembler for an out-of-tree target with InsnType bigger than 64 bits
from 4m30s to 48s.

D67686 did a similar thing for CodeEmitterGen.

The idea is to tweak the API of the APInt-like InsnType class so that
we don't need so many temporary InsnTypes. This takes advantage of the
rule stated in D52100 that currently "no string of bits extracted
from the encoding may exceeed 64-bits", so we can use uint64_t for some
temporaries.

D52100 goes on to say that "fields are still permitted to exceed 64-bits
so long as they aren't one contiguous string of bits". This patch breaks
that by always using a "uint64_t tmp" in the generated decodeToMCInst,
but it should be easy to fix in FilterChooser::emitBinaryParser by
choosing to use a different type of tmp based on the known total field
width.

Diff Detail

Event Timeline

foad requested review of this revision.Mar 5 2021, 7:37 AM
foad created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMar 5 2021, 7:37 AM
foad added a comment.Mar 5 2021, 7:48 AM

D52100 goes on to say that "fields are still permitted to exceed 64-bits
so long as they aren't one contiguous string of bits". This patch breaks
that by always using a "uint64_t tmp" in the generated decodeToMCInst,
but it should be easy to fix in FilterChooser::emitBinaryParser by
choosing to use a different type of tmp based on the known total field
width.

I didn't try to implement the fix because I don't have a target that needs it so I wouldn't be able to test it. @dsanders do you?

foad added a comment.Mar 16 2021, 10:17 AM

Ping! Does anyone have an interest in this for their own out-of-tree target?

I didn't try to implement the fix because I don't have a target that needs it so I wouldn't be able to test it. @dsanders do you?

Sorry, I've gone to try and confirm this a couple times in the last week and things kept coming up. I'll take another look a moment

I didn't try to implement the fix because I don't have a target that needs it so I wouldn't be able to test it. @dsanders do you?

Sorry, I've gone to try and confirm this a couple times in the last week and things kept coming up. I'll take another look a moment

We've got a few that reach 64 but AFAICT none that exceed it

dsanders accepted this revision.Mar 16 2021, 12:22 PM

LGTM

llvm/utils/TableGen/FixedLenDecoderEmitter.cpp
1125–1134

It looks like insertBits has implementations that cover both values of UseInsertBits. Would it make sense to always use insertBits here and rely on the type of tmp to pick the appropriate version of insertBits()?

This revision is now accepted and ready to land.Mar 16 2021, 12:22 PM
foad added inline comments.Mar 16 2021, 12:32 PM
llvm/utils/TableGen/FixedLenDecoderEmitter.cpp
1125–1134

Using insertBits relies on us having written the initial tmp = 0; line, which we prefer to avoid for the simple cases. I think that's the only reason not to use insertBits in all cases.

dsanders added inline comments.Mar 16 2021, 1:25 PM
llvm/utils/TableGen/FixedLenDecoderEmitter.cpp
1125–1134

That makes sense to me. Thanks