[CodeGen] Fix the computation of the alignment of split stores.

[courbet]

Summary

Right now the alignment of the lower half of a store is computed as
align/2, which fails for unaligned stores (align = 1), and is overly
pessimitic for, e.g. a 8 byte store aligned to 4 bytes.
Fixes PR44851

[courbet]

remove dplicate test

[gchatelet]

llvm/lib/CodeGen/CodeGenPrepare.cpp

6872

Can you reuse Alignment here?

[lebedev.ri]

llvm/lib/CodeGen/CodeGenPrepare.cpp

6873

On a first read, this is very confusing.
When splitting store in half, naturally one half will retain the alignment
of the original wider store, regardless of whether it was over-aligned or not,
while other will require adjustment.

6875

Can the no-longer-pessimism of the computation be demonstrated by the tests?

[courbet]

use opt test to explicitly show align

[courbet]

Add a test with align=2 to show "no-longer-pessimism".

[courbet]

Improve documentation as suggested during the review.

[courbet]

Thanks

[lebedev.ri]

llvm/test/CodeGen/X86/split-store-unaligned.ll

31

Hmm.
Could you please add a similar test (but with wide store being 64-bit aligned) with big-endian triple please.

[courbet]

rename test file

[courbet]

add PowerPC test

[lebedev.ri]

LG to me.

llvm/test/CodeGen/PowerPC/split-store-alignment.ll

2–3 ↗

(On Diff #243549)

Please unique checklines

; RUN: opt -S -mtriple=powerpc64le -codegenprepare -force-split-store < %s  | FileCheck --check-prefixes=ALL,PPC64LE %s
; RUN: opt -S -mtriple=powerpc64 -codegenprepare -force-split-store < %s  | FileCheck --check-prefixes=ALL,PPC64 %s

101–102 ↗

(On Diff #243549)

Okay, i guess there is no big-endian issue after all.

[courbet]

unique CHECK lines

[spatel]

llvm/test/CodeGen/PowerPC/split-store-alignment.ll

101–102 ↗

(On Diff #243549)

Wait - why is there no difference between LE and BE here?
We may need to explicitly specify the datalayout on the command-line or in the test file.

[spatel]

CGP is not creating an appropriate default datalayout based on triple, so we need to specify it explicitly:

$ opt -S splitstore.ll -mtriple=powerpc64 -data-layout="E" -force-split-store -codegenprepare
; ModuleID = 'splitstore.ll'
source_filename = "splitstore.ll"
target datalayout = "E"
target triple = "powerpc64"

define void @split_store_align8(i32 %x, i64* %p) {
  %z = zext i32 43 to i64
  %s = shl nuw nsw i64 %z, 32
  %z2 = zext i32 %x to i64
  %o = or i64 %s, %z2
  %1 = bitcast i64* %p to i32*
  %2 = getelementptr i32, i32* %1, i32 1
  store i32 %x, i32* %2, align 8
  %3 = bitcast i64* %p to i32*
  store i32 43, i32* %3, align 4
  ret void
}

$ opt -S splitstore.ll -mtriple=powerpc64le -data-layout="e" -force-split-store -codegenprepare
; ModuleID = 'splitstore.ll'
source_filename = "splitstore.ll"
target datalayout = "e"
target triple = "powerpc64le"

define void @split_store_align8(i32 %x, i64* %p) {
  %z = zext i32 43 to i64
  %s = shl nuw nsw i64 %z, 32
  %z2 = zext i32 %x to i64
  %o = or i64 %s, %z2
  %1 = bitcast i64* %p to i32*
  store i32 %x, i32* %1, align 8
  %2 = bitcast i64* %p to i32*
  %3 = getelementptr i32, i32* %2, i32 1
  store i32 43, i32* %3, align 4
  ret void
}

[courbet]

Add target triple in module in tests, which shows the brokenness of alignment
for BE targets. Fit it.

[courbet]

CGP is not creating an appropriate default datalayout based on triple, so we need to specify it explicitly:

Thanks !

[lebedev.ri]

LG for real now :)

llvm/test/CodeGen/PowerPC/split-store-alignment.ll

101–102 ↗

(On Diff #243549)

Oops, thanks for catching that.
So there was an issue after all.

[spatel]

LGTM - see inline for a comment nit and some potential test changes.

llvm/lib/CodeGen/CodeGenPrepare.cpp

6873

The suggested "one half" is better than the current "lower half" - eliminate the endian-specific inaccuracy.

llvm/test/CodeGen/PowerPC/split-store-alignment-le.ll

2 ↗

(On Diff #243750)

Unless I'm missing some advantage of the way of the tests are arranged currently, I prefer that:

1. The tests live under llvm/test/Transforms/CodeGenPrepare/<target>. I know there are exceptions, but that is where I first look for IR --> IR tests.
2. If we include the layout specifiers as parameters in the RUN line, then it saves some duplication, and it's easier to spot the endian differences.
3. If we can push the non-crashing tests as a preliminary commit, that would be nicer, so we can see the current buggy code (and the tests will remain just in case this patch ever gets reverted).

[lebedev.ri]

We probably want to backport the big-endian fix into 10.0 in some form?
Please do file a bug

[courbet]

rebase on submitted base tests.

[courbet]

Thanks for the review !

[hans]

In D74311#1870555, @lebedev.ri wrote:

We probably want to backport the big-endian fix into 10.0 in some form?
Please do file a bug

Was there a bug filed for this yet?

[lebedev.ri]

In D74311#1871706, @hans wrote:

In D74311#1870555, @lebedev.ri wrote:

We probably want to backport the big-endian fix into 10.0 in some form?
Please do file a bug

Was there a bug filed for this yet?

#44877

[hans]

In D74311#1871709, @lebedev.ri wrote:

In D74311#1871706, @hans wrote:

In D74311#1870555, @lebedev.ri wrote:

We probably want to backport the big-endian fix into 10.0 in some form?
Please do file a bug

Was there a bug filed for this yet?

#44877

Thanks! I'll follow up there.

llvm/lib/CodeGen/CodeGenPrepare.cpp

auto CreateSplitStore = [&](Value *V, bool Upper) {
     V = Builder.CreateZExtOrBitCast(V, SplitStoreType);
     Value *Addr = Builder.CreateBitCast(
         SI.getOperand(1),
         SplitStoreType->getPointerTo(SI.getPointerAddressSpace()));
     if ((IsLE && Upper) || (!IsLE && !Upper))
       Addr = Builder.CreateGEP(
           SplitStoreType, Addr,
           ConstantInt::get(Type::getInt32Ty(SI.getContext()), 1));
-    Builder.CreateAlignedStore(V, Addr,
-                               Upper ? SI.getAlign() / 2 : SI.getAlign());
+    MaybeAlign Alignment = SI.getAlign();
+    if (Upper && Alignment) {
+      // When the original store is aligned, the lower part has the same alignment.
+      // Find the best alignment for the upper part. 
+      Alignment = commonAlignment(Alignment, HalfValBitSize / 8);
+    }
+    Builder.CreateAlignedStore(V, Addr, Alignment);
   };
 
   CreateSplitStore(LValue, false);
   CreateSplitStore(HValue, true);
 
   // Delete the old store.
   SI.eraseFromParent();
   return true;

llvm/test/CodeGen/X86/split-store-unaligned.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -codegenprepare -mtriple=x86_64-unknown-unknown -force-split-store -S < %s | FileCheck %s
+
+target datalayout = "e-m:x-p:32:32-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:32-n8:16:32-a:0:32-S32"
+target triple = "i686-w64-windows-gnu"
+
+define void @ttml_read_coords(float %x, i64* %p) {
+; CHECK-LABEL: @ttml_read_coords(
+; CHECK-NEXT:    [[B:%.*]] = bitcast float [[X:%.*]] to i32
+; CHECK-NEXT:    [[Z:%.*]] = zext i32 0 to i64
+; CHECK-NEXT:    [[S:%.*]] = shl nuw nsw i64 [[Z]], 32
+; CHECK-NEXT:    [[Z2:%.*]] = zext i32 [[B]] to i64
+; CHECK-NEXT:    [[O:%.*]] = or i64 [[S]], [[Z2]]
+; CHECK-NEXT:    [[TMP1:%.*]] = bitcast i64* [[P:%.*]] to i32*
+; CHECK-NEXT:    store i32 [[B]], i32* [[TMP1]], align 1
+; CHECK-NEXT:    [[TMP2:%.*]] = bitcast i64* [[P]] to i32*
+; CHECK-NEXT:    [[TMP3:%.*]] = getelementptr i32, i32* [[TMP2]], i32 1
+; CHECK-NEXT:    store i32 0, i32* [[TMP3]], align 1
+; CHECK-NEXT:    ret void
+;
+  %b = bitcast float %x to i32
+  %z = zext i32 0 to i64
+  %s = shl nuw nsw i64 %z, 32
+  %z2 = zext i32 %b to i64
+  %o = or i64 %s, %z2
+  store i64 %o, i64* %p, align 1
+  ret void
+}