Page MenuHomePhabricator

Fix compile-time regression caused by rL371928
ClosedPublic

Authored by dsanders on Sep 17 2019, 5:58 PM.

Details

Summary

Also fixup rL371928 for cases that occur on our out-of-tree backend

There were still quite a few intermediate APInts and this caused the
compile time of MCCodeEmitter for our target to jump from 16s up to
~5m40s. This patch, brings it back down to ~17s by eliminating pretty
much all of them using two new APInt functions (extractBitsAsZExtValue(),
insertBits() but with a uint64_t). The exact conditions for eliminating
them is that the field extracted/inserted must be <=64-bit which is
almost always true.

Note: The two new APInt API's assume that APInt::WordSize is at least
64-bit because that means they touch at most 2 APInt words. They
statically assert that's true. It seems very unlikely that someone
is patching it to be smaller so this should be fine.

Diff Detail

Repository
rL LLVM

Event Timeline

dsanders created this revision.Sep 17 2019, 5:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 17 2019, 5:59 PM
jmolloy accepted this revision.Sep 18 2019, 1:35 AM

Thankyou Daniel! Looks great!

One question though, how did your out-of-tree backend trigger this case? It only gets enabled with BitWidth > 64 which was totally not supported before, so surely you would have had a broken encoder?

Cheers,

James

This revision is now accepted and ready to land.Sep 18 2019, 1:35 AM

Thankyou Daniel! Looks great!

One question though, how did your out-of-tree backend trigger this case? It only gets enabled with BitWidth > 64 which was totally not supported before, so surely you would have had a broken encoder?

Cheers,

James

Thanks.

Historically, we let tablegen give us the first 64-bits and then tacked on extra bytes via handwritten C++ (which is as error prone as you'd expect). A while back we made Inst field reflect the whole instruction and added >64-bit support to the disassembler but we didn't have time to do the encoder at the same time. We therefore added a quick hack to have the CodeEmitterGen discard the definition for bit 64 and up and filed a bug report to come back to it. The end result was that when your patch landed we got the APInt version because BitWidth was >64 in the early test but it still only generated the first 64-bits because of our hack. The next step for us now that we've updated our MCCodeEmitter to the APInt versions is to remove our hack and let tblgen handle it all :-)

Ah great, thanks for the context, and I'm glad this patch helps you out!

This revision was automatically updated to reflect the committed changes.