This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][FastISel] Handle CRC32 intrinsics
ClosedPublic

Authored by aengelke on Apr 21 2023, 5:27 AM.

Details

Summary

With a similar reason as D148023; some applications make heavy use of
the CRC32 intrinsic (e.g., as part of a hash function) and therefore
benefit from avoiding frequent SelectionDAG fallbacks. In our
application, we get a 2% compile-time improvement.

Diff Detail

Event Timeline

aengelke created this revision.Apr 21 2023, 5:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2023, 5:27 AM
aengelke requested review of this revision.Apr 21 2023, 5:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2023, 5:27 AM
aengelke updated this revision to Diff 515689.Apr 21 2023, 5:28 AM

(format changes)

Note that I'm not entirely happy with duplicating the test cases; changing the existing cases in arm64-crc32.ll could also work, but currently generates an extra uxtb/uxth with FastISel. Let me know what you think.

aengelke updated this revision to Diff 515706.Apr 21 2023, 5:56 AM

Add check whether subtarget actually supports CRC.

There's been very little development of AArch64 FastISel recently because we've turned on GlobalISel by default for AArch64. Is there some reason you're not using it? Or are you getting globalisel fallbacks so frequently that hacking fastisel is somehow worthwhile?

We care a lot about compile times and in our application, GlobalISel (when it succeeds, about 10% fallback rate) increases compile times by ~50% compared to FastISel. We do plan to look at GlobalISel performance at some point, but it seems like there's a lot of work to be done to bring it at least on-par with FastISel wrt. compile times.

Okay. I'm bringing it up because I'd like to make sure fastisel remains limited in scope; we don't have all the GlobalISel infrastructure, so reviewing any additions is significantly more work. (For comparison, globalisel supports these intrinsics with zero globalisel-specific lines of code.)

llvm/lib/Target/AArch64/AArch64FastISel.cpp
3792

You can assume intrinsics have the type declared in IntrinsicsAArch64.td; I don't think checking the type is legal does anything here.

3831

I'm pretty sure fastEmitInst_rr can't fail, so there isn't any reason to null-check the return value. Is this pattern copied from somewhere?

aengelke updated this revision to Diff 517170.Apr 26 2023, 7:38 AM

Remove unnecessary checks

aengelke marked 2 inline comments as done.Apr 26 2023, 7:45 AM

Okay. I'm bringing it up because I'd like to make sure fastisel remains limited in scope; we don't have all the GlobalISel infrastructure, so reviewing any additions is significantly more work.

I totally understand, and I don't plan further substantial additions (but crc32 and struct returns are fairly important for us).

llvm/lib/Target/AArch64/AArch64FastISel.cpp
3831

I probably slightly mixed this up with fastEmit_* which can fail (see sqrt intrinsic) and two occurences of fastEmitInst have checks (selectSDiv)/assertions (Intrinsic::frameaddress).

aengelke updated this revision to Diff 517620.Apr 27 2023, 9:55 AM

(no change; re-trigger CI)

This revision is now accepted and ready to land.Apr 27 2023, 11:02 AM
This revision was automatically updated to reflect the committed changes.