This is an archive of the discontinued LLVM Phabricator instance.

Have DataLayout::isLegalInteger accept a uint64_t
ClosedPublic

Authored by mkuper on Mar 17 2016, 4:58 PM.

Details

Summary

Integer type sizes may have up to IntegerType::MAX_INT_BITS, which is currently 1<<23, so nothing that fits into an unsigned int will not be legal. However, having a uint64_t passed here avoids bugs due to truncation from uint64_t, as in PR26972.

Alternatively, we can require always checking for < UINT_MAX (or, better yet, for < MAX_INT_BITS) explicitly before calling DataLayout::isLegalInteger(), but that would make code like "DL.isLegalInteger(DL.getTypeSizeInBits(Ty))" (which we currently have in several places) wrong.

Diff Detail

Event Timeline

mkuper updated this revision to Diff 50989.Mar 17 2016, 4:58 PM
mkuper retitled this revision from to Have DataLayout::isLegalInteger accept a uint64_t.
mkuper updated this object.
mkuper added reviewers: spatel, mehdi_amini.
mkuper added a subscriber: llvm-commits.
mehdi_amini accepted this revision.Mar 17 2016, 5:02 PM
mehdi_amini edited edge metadata.

LGTM with a nit below.

include/llvm/IR/DataLayout.h
239

I guess the case where Width > MAX_INT_BITS is rare enough that it's not worth an early exit here?

test/Transforms/SROA/pr26972.ll
10

Add a comment specifying that this constant is intended to trigger 32 bits overflow.

This revision is now accepted and ready to land.Mar 17 2016, 5:02 PM

Thanks, Mehdi!

include/llvm/IR/DataLayout.h
239

Yeah, had the same thought, I think it's most probably not worth it.

test/Transforms/SROA/pr26972.ll
10

Right, thanks!

This revision was automatically updated to reflect the committed changes.