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

[asb]

Summary

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.

[asb]

Updated patch to include full context.

[efriedma]

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.

clang/lib/CodeGen/CGRecordLayoutBuilder.cpp

414

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]

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

@efriedma: was your comment an LGTM?

[efriedma]

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

[asb]

In D79155#2088191, @efriedma wrote:

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.

[dmgreen]

Hello. We've received reports that this is bloating codesize of some code, quite a lot in places. There is an example in https://godbolt.org/z/66TEKa1xK. Essentially the glomming together of reads/writes into i32's (in our case) helps to reduce the total number of loads/stores needed. Splitting that up into individual i8/i16's creates a lot more load/mask/load/mask/or/store sequences.

I suspect it's better in cases, worse in others. It depends on where exactly the bitfield delimitators lie. And whether they are more likely to be on i32/i64 boundaries (which I believe is common in a lot of cases bitfields are used in), or to be more randomly distributed.

clang/lib/CodeGen/CGRecordLayoutBuilder.cpp

if (isDiscreteBitFieldABI()) {
     return;
   }
 
   // Check if OffsetInRecord is better as a single field run. When OffsetInRecord
   // has legal integer width, and its bitfield offset is naturally aligned, it
   // is better to make the bitfield a separate storage component so as it can be
   // accessed directly with lower cost.
   auto IsBetterAsSingleFieldRun = [&](uint64_t OffsetInRecord,
                                       uint64_t StartBitOffset) {
     if (!Types.getCodeGenOpts().FineGrainedBitfieldAccesses)
       return false;
-    if (!DataLayout.isLegalInteger(OffsetInRecord))
+    if (OffsetInRecord < 8 || !llvm::isPowerOf2_64(OffsetInRecord) ||
+        !DataLayout.fitsInLegalInteger(OffsetInRecord))
       return false;
     // Make sure StartBitOffset is natually aligned if it is treated as an
     // IType integer.
      if (StartBitOffset %
             Context.toBits(getAlignment(getIntNType(OffsetInRecord))) !=
         0)
       return false;
     return true;

clang/test/CodeGenCXX/finegrain-bitfield-type.cpp

 // RUN: %clang_cc1 -triple x86_64-linux-gnu -ffine-grained-bitfield-accesses \
 // RUN:   -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple riscv64-linux-gnu -ffine-grained-bitfield-accesses \
+// RUN:   -emit-llvm -o - %s | FileCheck %s
+
+// Note: This test checks the X86-64 and RISC-V targets in order to explore
+// behaviour when i8/i16 are native integer widths (X86-64) and when they're
+// not (RISC-V).
+
 struct S4 {
   unsigned long f1:28;
   unsigned long f2:4;
   unsigned long f3:12;
 };
 struct S4 a4;
 
 struct S5 {
   unsigned long f1:28;
   unsigned long f2:4;
   unsigned long f3:28;
   unsigned long f4:4;
   unsigned long f5:12;
 };
 struct S5 a5;
 
 // CHECK: %struct.S4 = type { i32, i16 }
 // CHECK-NOT: %struct.S4 = type { i48 }
 // CHECK: %struct.S5 = type { i32, i32, i16, [6 x i8] }
-// CHECK-NOT: %struct.S5 = type { i80 }
+// CHECK-NOT: %struct.S5 = type { i80 }