Page MenuHomePhabricator

[CodeGen] Increase applicability of ffine-grained-bitfield-accesses for targets with limited native integer widths

Authored by asb on Apr 30 2020, 12:17 AM.



As pointed out in PR45708, -ffine-grained-bitfield-accesses doesn't trigger in all cases you think it might for RISC-V. The logic in CGRecordLowering::accumulateBitFields checks OffsetInRecord is a legal integer according to the datalayout. RISC targets will typically only have the native width as a legal integer type so this check will fail for OffsetInRecord of 8 or 16 when you would expect the transformation is still worthwhile.

This patch changes the logic to check for an OffsetInRecord of a at least 1 byte, that fits in a legal integer, and is a power of 2. We would prefer to query whether native load/store operations are available, but I don't believe that is possible.

Diff Detail

Event Timeline

asb created this revision.Apr 30 2020, 12:17 AM
asb added a subscriber: apazos.Apr 30 2020, 12:18 AM
asb updated this revision to Diff 261138.Apr 30 2020, 12:43 AM

Updated patch to include full context.

The -ffine-grained-bitfield-accesses seems to generate weird results in certain cases. For example, on x86 we generate an unaligned load in the following example.

struct S  { long c : 8; long z: 24; long : 0; };
struct S s;
int f() { return s.c+s.z; }

I guess that's not really an issue with this patch specifically, though; the general algorithm is just sort of weird.


StartBitOffset is the offset in bits of the beginning of the current run, the comment above explains that. It's not obvious what OffsetInRecord is; I guess it's the size in bits of the current run? The name isn't really intuitive.

This patch seems reasonable; you want to allow any legal integer, and any smaller type that can probably be naturally loaded and stored.

asb added a comment.Jun 10 2020, 10:45 PM

Ping on this. The patch still applies cleanly against current HEAD.

@efriedma: was your comment an LGTM?

Please add a comment explaining what OffsetInRecord means; then LGTM.

asb added a comment.Jun 12 2020, 1:30 AM

Please add a comment explaining what OffsetInRecord means; then LGTM.

Thanks. It's not easy to follow, but having stepped through it I agree with yu that it is the size in bits of the current run. I'll add that comment and commit.

This revision was not accepted when it landed; it landed in state Needs Review.Jun 12 2020, 2:40 AM
This revision was automatically updated to reflect the committed changes.