[AArch64] Change the preferred alignment for char and short

[evandro]

Summary

By aligning small variables to word alignment, I noticed modest overall improvements between 1 and 2% with just a few modest regressions.

[t.p.northover]

Tests?

[rengolin]

What about code size?

[t.p.northover]

And what CPU did you run the tests on? As far as I know the larger cores don't care about alignment as long as a cache line isn't crossed (which it won't be for the natural alignment).

[evandro]

What about code size?

I observed an insignificant (<0.01%) increase in the code size.

[evandro]

In D21414#459420, @t.p.northover wrote:

And what CPU did you run the tests on? As far as I know the larger cores don't care about alignment as long as a cache line isn't crossed (which it won't be for the natural alignment).

That's the problem that I noticed, especially with some variables being aggressively accessed in wider chunks (often by library code) crossing cache lines.

Though I'd expect that 64-bit alignment would be the preferred alignment in AArch64, any difference from 32-bit alignment was overall too small to notice.

I tested these changes only on Cortex A57 and Exynos M1 and both benefit from this preferred alignment (depending on the benchmark, A57 more so than M1).

[evandro]

Tests?

Will add a test when updating the patch or before committing it.

[evandro]

Added a test case.

[mcrosier]

Hi Evandro,
Who approved this patch?

Also, we're seeing a ~1.8% regression in SPEC2006/perlbench on Kryo. Runs for the other SPEC2006 benchmarks are in flight. What workloads did you use for testing?

Chad

[rengolin]

In D21414#476803, @mcrosier wrote:

Also, we're seeing a ~1.8% regression in SPEC2006/perlbench on Kryo. Runs for the other SPEC2006 benchmarks are in flight. What workloads did you use for testing?

I haven't approved yet, nor I've seen Tim do it. I think we better revert it for now until further clarifications come to light.

cheers,
--renato

[mcrosier]

In D21414#476804, @rengolin wrote:

I haven't approved yet, nor I've seen Tim do it. I think we better revert it for now until further clarifications come to light.

Sounds reasonable to me. Would you like to revert or shall I, Renato?

[rengolin]

Meetings all day, feel free to revert, pelase.

[evandro]

Folks,

I took it that Tim approved this patch when he said in an email: "I'm happy for you to go ahead."

[rengolin]

In D21414#476827, @evandro wrote:

I took it that Tim approved this patch when he said in an email: "I'm happy for you to go ahead."

Sorry Evandro, I forgot about that email, now I remembered.

Still, this was a contentious issue, and it did have some bad reactions. I think we need more testing and benchmarking.

At least now we have more data to discuss it on.

cheers,
--renato

[evandro]

I tested on both M1 and A57 using SPEC CPU2000 and proprietary benchmarks. I noticed minor regressions and significant improvements on both targets

[evandro]

By all means, let us discuss it further before reverting this patch.

[mcrosier]

In D21414#476842, @evandro wrote:

I tested on both M1 and A57 using SPEC CPU2000 and proprietary benchmarks. I noticed minor regressions and significant improvements on both targets

I've reverted the commits in r274766-r274768. Sorry about that, Evandro. I didn't know Tim approved via email. We should have Kryo numbers shortly, if you can wait.

[evandro]

In D21414#476855, @mcrosier wrote:

I've reverted the commits in r274766-r274768. Sorry about that, Evandro. I didn't know Tim approved via email. We should have Kryo numbers shortly, if you can wait.

You don't just revert a two weeks old patch BEFORE tests for one target are in and BEFORE comparing its results with other targets.

The burden of proof now lied with you to revert the patch, but your reverting it put the burden back on me.

Make no mistake, I do not appreciate that.

[rengolin]

My apologies Evandro. I know we have a policy that approved patches on the list are also valid, and that this review not being in the "approve" status doesn't count. I should have searched the list for more information.

Chad, maybe re-instate the patch and discuss this on the list would be the best way forward now.

cheers,
--renato

[mcrosier]

In D21414#477024, @evandro wrote:

In D21414#476855, @mcrosier wrote:

I've reverted the commits in r274766-r274768. Sorry about that, Evandro. I didn't know Tim approved via email. We should have Kryo numbers shortly, if you can wait.

You don't just revert a two weeks old patch BEFORE tests for one target are in and BEFORE comparing its results with other targets.

Please understand the reason I reverted the patch was because I didn't see formal approval. That alone is justification enough.

I apologize I missed Tim's email; I did search the list for an informal LGTM, but I found nothing.

Make no mistake, I do not appreciate that.

Understandable.. Evandro, please make the assumption that everyone is trying to do the right thing. I've seen patches committed without proper approval on too many occasion; I sincerely apologize if you we're caught in the crossfire.

The final results for Spec2006 on Kryo were a net gain of 0.2% geomean, so thank you. That being said, I'll go ahead a recommit the patches.

Chad

[evandro]

@mcrosier,

Indeed, too many assumptions were made here. However, we can certainly agree that not waiting even 15min for a reply of the author of the patch, myself, was precipitated. Hopefully, in the future, we'll be able to avoid such actions on anecdotal information.

I'm glad to learn that the results show benefits on Kryo too.

Thanks for reinstating the patches.

[mssimpso]

I think some of the confusion here may have been due to the fact that llvm-commits wasn't added as a subscriber to the review. Since it wasn't on the list, few people were probably aware of the patch or its approval. Glad everything is resolved now!

llvm/trunk/lib/Target/AArch64/AArch64TargetMachine.cpp

static std::unique_ptr<TargetLoweringObjectFile> createTLOF(const Triple &TT) {
   return make_unique<AArch64_ELFTargetObjectFile>();
 }
 
 // Helper function to build a DataLayout string
 static std::string computeDataLayout(const Triple &TT, bool LittleEndian) {
   if (TT.isOSBinFormatMachO())
     return "e-m:o-i64:64-i128:128-n32:64-S128";
   if (LittleEndian)
-    return "e-m:e-i64:64-i128:128-n32:64-S128";
-  return "E-m:e-i64:64-i128:128-n32:64-S128";
+    return "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128";
+  return "E-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128";
 }
 
 // Helper function to set up the defaults for reciprocals.
 static void initReciprocals(AArch64TargetMachine& TM, AArch64Subtarget& ST)
 {
   // For the estimates, convergence is quadratic, so essentially the number of
   // digits is doubled after each iteration. ARMv8, the minimum architected
   // accuracy of the initial estimate is 2^-8.  Therefore, the number of extra

llvm/trunk/test/CodeGen/AArch64/preferred-alignment.ll

+; RUN: llc -mtriple=aarch64 -O0 < %s | FileCheck %s
+
+; Function Attrs: nounwind
+define i32 @foo() #0 {
+entry:
+  %c = alloca i8, align 1
+; CHECK:  add x0, sp, #12
+  %s = alloca i16, align 2
+; CHECK-NEXT: add x1, sp, #8
+  %i = alloca i32, align 4
+; CHECK-NEXT: add x2, sp, #4
+  %call = call i32 @bar(i8* %c, i16* %s, i32* %i)
+  %0 = load i8, i8* %c, align 1
+  %conv = zext i8 %0 to i32
+  %add = add nsw i32 %call, %conv
+  %1 = load i16, i16* %s, align 2
+  %conv1 = sext i16 %1 to i32
+  %add2 = add nsw i32 %add, %conv1
+  %2 = load i32, i32* %i, align 4
+  %add3 = add nsw i32 %add2, %2
+  ret i32 %add3
+}
+
+declare i32 @bar(i8*, i16*, i32*) #1
+
+attributes #0 = { nounwind "no-frame-pointer-elim"="false" }
+attributes #1 = { "no-frame-pointer-elim"="false" }
+