Page MenuHomePhabricator

Fix bitwidth for x87 extended-precision floating-point type
Needs ReviewPublic

Authored by ddcc on Nov 21 2016, 8:28 PM.

Details

Summary

llvm::APFloat::x87DoubleExtended is defined as having 80 bits of size

Event Timeline

ddcc updated this revision to Diff 78825.Nov 21 2016, 8:28 PM
ddcc retitled this revision from to Fix bitwidth for x87 extended-precision floating-point type.
ddcc updated this object.
ddcc added a reviewer: rsmith.
ddcc added a subscriber: cfe-commits.
ddcc added a comment.Nov 21 2016, 8:30 PM

I could be completely mistaken here, but currently Ctx.getRealTypeForBitwidth(llvm::APFloat::getSizeInBits(llvm::APFloat::x87DoubleExtended)) with a ASTContext Ctx does not round-trip correctly.

bruno added a subscriber: bruno.Nov 28 2016, 12:56 PM
bruno added inline comments.
lib/Basic/TargetInfo.cpp
229

The change makes sense but I believe there's some historical reason why this is 96 instead of 80? What problem have you found in practice? Do you have a testcase or unittest that exercise the issue (and that could be added to the patch)?

ddcc added inline comments.Nov 28 2016, 1:45 PM
lib/Basic/TargetInfo.cpp
229

I'd be curious why it was historically set to 96; I dug up rL190044 as the original commit, but it doesn't mention 80 vs 96 for this at all.

I've been implementing an alternative symbolic constraint solver backend for the static analyzer, including floating-point support, which performs some type conversion and needs to reason about bitwidth. I'm still working on those patches, since they touch a lot of code and currently break some tests. I can write up a testcase for this issue, though I've only written IR testcases before and I'm not sure how I'd directly call a clang internal function?

efriedma added inline comments.
lib/Basic/TargetInfo.cpp
229

It was set to 96 because that's what the only caller (handleModeAttr) expects; see https://reviews.llvm.org/rL65935 and https://reviews.llvm.org/rL190926. It's arbitrary as far as I know.

ddcc updated this revision to Diff 79996.Dec 1 2016, 4:19 PM

Change definition

There's nothing wrong with this change, as far as I can tell. That said, if you're planning to use getRealTypeByWidth anywhere other than AddModeAttr, it's probably a bug; the behavior of getRealTypeByWidth isn't useful for any other purpose given that clang supports multiple 128-bit floating-point types.