This is an archive of the discontinued LLVM Phabricator instance.

[APInt] Add a concat method, use LLVM_UNLIKELY to help optimizer.
ClosedPublic

Authored by lattner on Sep 10 2021, 11:31 AM.

Details

Summary

Three unrelated changes:

  1. Add a concat method as a convenience to help write bitvector use cases in a nicer way.
  2. Use LLVM_UNLIKELY as suggested by @xbolva00 in a previous patch.
  3. Fix casing of some "slow" methods to follow naming standards.

Diff Detail

Event Timeline

lattner created this revision.Sep 10 2021, 11:31 AM
lattner requested review of this revision.Sep 10 2021, 11:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 10 2021, 11:31 AM
lattner updated this revision to Diff 371990.Sep 10 2021, 12:45 PM

Capitalize identifiers to follow prevailing convention, and fix a comment typo. NFC.

craig.topper added inline comments.Sep 10 2021, 1:45 PM
llvm/include/llvm/ADT/APInt.h
351

This is may be too subtle, but we can avoid a branch here if we use.

U.VAL == WORDTYPE_MAX >> ((APINT_BITS_PER_WORD - BitWidth) & 0x3f);

That will alias 64 and 0 to both use WORD_TYPE for the compare. Since U.VAL should always be 0 for BitWidth == 0. The compare would always fail.

llvm/lib/Support/APInt.cpp
344

I was wondering if insertBits would be better here, but the last resort code in that resorts to copying bit by bit. But it would avoid a second heap allocation for widening the LSB. So maybe worth improving insertBits.

lattner added inline comments.Sep 10 2021, 1:57 PM
llvm/include/llvm/ADT/APInt.h
351

I like it, I'll pull this in.

llvm/lib/Support/APInt.cpp
344

I didn't spend much time worrying about this, but I'll look at improving this. Thanks!

lattner updated this revision to Diff 372021.Sep 10 2021, 3:24 PM

Incorporate Craig's feedback: Optimize isAllOnes and concatSlowCase

Also change more variable names to appease clang-tidy.

foad added a subscriber: foad.Sep 13 2021, 6:17 AM
foad added inline comments.
llvm/include/llvm/ADT/APInt.h
883

Is there a reason for the argument to be NewLSB instead of NewMSB?

If there was a non-member form would it be static APInt concat(const APInt &MSB, const APInt &LSB)? I would slightly prefer a (LSB, MSB) argument order, but I guess there is no real consistency in existing APIs.

llvm/lib/Support/APInt.cpp
345

Larger than what? Did you mean "large"?

lattner marked an inline comment as done.Sep 13 2021, 9:04 AM
lattner added inline comments.
llvm/include/llvm/ADT/APInt.h
883

Yes, check out the unit tests. I actually started this way, but it was too weird for:

APInt(0xA).concat(APInt(0xB)).concat(APInt(0xC)) == 0xCBA

Given how dot syntax works, it is better for this to be NewLSB.

llvm/lib/Support/APInt.cpp
345

Yes, thank you for catching that. Fixed to "large".

lattner updated this revision to Diff 372273.Sep 13 2021, 9:04 AM
lattner marked an inline comment as done.

Comment typo fix, NFC.

lattner accepted this revision.Sep 13 2021, 9:23 AM

I don't hear any concerns, I'll plan to land this later today unless anyone has an objection - thanks!

This revision is now accepted and ready to land.Sep 13 2021, 9:23 AM