Page MenuHomePhabricator

[AST] Unpack FPFeatures bits to BinaryOperator, NFC.
AbandonedPublic

Authored by hokein on Mar 2 2020, 3:29 AM.

Details

Reviewers
sammccall
Summary

The stmt bit-fields is full (max 64 bits) for BinaryOperator now, adding
a new bit field (error) causes an 'static_assert(sizeof(*this) <=8)'
violation in Stmt constructor.

This patch unpacks the FPFeautres, make available bitfields for error
bit (https://reviews.llvm.org/D65591).

Diff Detail

Event Timeline

hokein created this revision.Mar 2 2020, 3:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2020, 3:29 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
hokein edited the summary of this revision. (Show Details)Mar 2 2020, 3:29 AM

I'm working on a patch that will move FPOptions to Trailing storage, part of https://reviews.llvm.org/D72841 ; hope to have another revision uploaded today or tomorrow. I got feedback that moving the field to BinaryOperator isn't adequate.

As discussed offline... this undoes a desirable optimization from D54526.

A couple of possible ways to avoid this:

  • FPOptions integer encoding can fit into 7 bits: combine integer behavior + exception behavior into 3 x 5 = 15 states that fits in 4 bits.
  • IsOMPStructuredBlock bit is basically unused (just for AST dump and an unused ASTMatcher), that bit could be dropped

Former is probably the eaisest.

hokein updated this revision to Diff 249606.Mar 11 2020, 6:26 AM

reduce FPOption from 8 bits to 7 bits, based on the offline discussion.

hokein updated this revision to Diff 249607.Mar 11 2020, 6:32 AM

update a missing CXXOperatorCallExprBitfields, and fix typos.

I'm working on a patch that will move FPOptions to Trailing storage, part of https://reviews.llvm.org/D72841 ; hope to have another revision uploaded today or tomorrow. I got feedback that moving the field to BinaryOperator isn't adequate.

Great! Moving FPOptions to trailing storage (which I guess allows it to only be present for floating point ops?) is much better than making it a member, and frees up a lot of bits.

To unblock adding the extra "error" bit that we need (which really does apply to all Exprs), we could do any of:

  1. wait for D72841 to land, including the trailingstorage
  2. move the trailing-storage parts out of that patch, and do it first as a smaller separate change
  3. find some other way to save the bit for now (e.g. by encoding rounding + exceptions together), and undo that once saving 1 bit doesn't matter

Any objections if we do 3 and take care of rolling it back later?

clang/include/clang/Basic/LangOptions.h
200

this is implied and unneccesary

358–359

Making use of the encoding of packed in lots of places is fragile and hard to maintain.

I'd suggest the private primitives:
setRoundingAndExceptionMode(FPRoundingModeKind, FPExceptionModeKind)
getRoundingAndExceptionMode() -> pair<FPRoundingModeKind, FPExceptionModeKind>
and implementing everything in terms of them.

458

FIXME: unpack this once saving one bit isn't critical here.

460

rounding_and_exceptions?

hokein updated this revision to Diff 249613.Mar 11 2020, 7:10 AM
hokein marked 4 inline comments as done.

address comments.

I'm working on a patch that will move FPOptions to Trailing storage, part of https://reviews.llvm.org/D72841 ; hope to have another revision uploaded today or tomorrow. I got feedback that moving the field to BinaryOperator isn't adequate.

Great! Moving FPOptions to trailing storage (which I guess allows it to only be present for floating point ops?) is much better than making it a member, and frees up a lot of bits.

To unblock adding the extra "error" bit that we need (which really does apply to all Exprs), we could do any of:

  1. wait for D72841 to land, including the trailingstorage
  2. move the trailing-storage parts out of that patch, and do it first as a smaller separate change
  3. find some other way to save the bit for now (e.g. by encoding rounding + exceptions together), and undo that once saving 1 bit doesn't matter

    Any objections if we do 3 and take care of rolling it back later?

I think waiting for 1 would probably take a long time since that is a huge patch. I'd prefer 3 to unblock the "error" bit stuff, but I'm also happy with 2. what's your thought @mibintc?

well, it would probably be quickest to separate the trailing storage into a separate patch. i'll try to get that submitted today. shall i submit that as a phabricator review or is there a different, preferred way to do that?

this looks reasonable to me, would like to give @mibintc and also @sepavloff a chance to chime in.

clang/include/clang/Basic/LangOptions.h
437

This assertion seems weird, and it doesn't test much - new enum values are typically added at the end.

460

nit: NumExceptionModes ?
(It's not the max value, it's max + 1)

well, it would probably be quickest to separate the trailing storage into a separate patch. i'll try to get that submitted today. shall i submit that as a phabricator review or is there a different, preferred way to do that?

(Sorry our comments keep racing)

If you don't mind doing that, it'd be great - phabricator is the best way.
I'm happy to do my best reviewing, but haven't made a similar change myself, so getting rjmccall's eyes on it would definitely get you a better check.

well, it would probably be quickest to separate the trailing storage into a separate patch. i'll try to get that submitted today. shall i submit that as a phabricator review or is there a different, preferred way to do that?

that sounds good to me, thanks!

sepavloff added inline comments.Mar 11 2020, 8:18 AM
clang/include/clang/Basic/LangOptions.h
461

There are 5 rounding directions defined by IEEE-754, which we should eventually support. Some targets supports additional rounding modes, which we might want to keep in FP environment. Existing implementation uses 3 exception behavior modes and we could want to extend them. For example it makes sense to introduce a special mode where Inexact exception is ignored but others are maintained. Four bits are too small space for FP environment.

I ran into an obstacle trying to add Trailing storage onto binary operator. it will probably take me longer.

I ran into an obstacle trying to add Trailing storage onto binary operator. it will probably take me longer.

sure, no worries, and thanks for your help. It is not super urgent for us at the moment (we have other patches under review).

nridge added a subscriber: nridge.Mar 12 2020, 6:54 AM
hokein abandoned this revision.Mar 17 2020, 2:06 AM

woo, looks like the IsOMPStructuredBlock bit in Stmt was removed in https://reviews.llvm.org/rGd5edcb90643104d6911da5c0ff44c4f33fff992f, we can use the bit for error, so this patch is not needed anymore.