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

Repository
rL LLVM

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 ↗(On Diff #50989)

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 ↗(On Diff #50989)

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 ↗(On Diff #50989)

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

test/Transforms/SROA/pr26972.ll
10 ↗(On Diff #50989)

Right, thanks!

This revision was automatically updated to reflect the committed changes.