Page MenuHomePhabricator

[PowerPC][AIX] Enable Shrinkwrapping on 32 and 64 bit AIX.
ClosedPublic

Authored by sidbav on Jan 20 2021, 2:19 PM.

Details

Summary

Currently Shrinkwrap is not enabled on AIX.
This patch enables shrink wrap on 32 and 64 bit AIX, and 64 bit ELF.

Diff Detail

Event Timeline

sidbav created this revision.Jan 20 2021, 2:19 PM
sidbav requested review of this revision.Jan 20 2021, 2:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2021, 2:19 PM
jsji added a reviewer: Restricted Project.Jan 20 2021, 2:24 PM
jsji added a subscriber: jsji.

Please add testcases.

sidbav updated this revision to Diff 318015.Jan 20 2021, 2:28 PM

Please add testcases.

Any PPC64/PPC64LE test cases for shrink-wrapping can be updated to have a couple of new RUN lines (for 32-bit and 64-bit AIX).

sidbav updated this revision to Diff 319386.Jan 26 2021, 12:41 PM

Add test cases for 32 and 64 bit AIX

sfertile added inline comments.Wed, Feb 10, 7:29 AM
llvm/test/CodeGen/PowerPC/ppc-shrink-wrapping.ll
3

A lot of lines in this patch add whitespace error, I get the following message when applying this patch with git:

warning: squelched 28 whitespace errors
warning: 33 lines add whitespace errors.

This line adds one, but there are too many to call out each individually on phab. Can you open the files that were modified in this patch in vim and highlight trailing whitespace and remove it on any line that was modified?

sidbav updated this revision to Diff 322686.Wed, Feb 10, 7:47 AM

Remove trailing whitespace

sidbav marked an inline comment as done.Wed, Feb 10, 7:47 AM
sfertile added inline comments.Wed, Feb 10, 8:20 AM
llvm/test/CodeGen/PowerPC/shrink-wrap.ll
2

When there are more then one check-prefix, then we use --check-prefixes=AIX,32AIX instead of using '-check-prefix' multiple times.

2

Both the new run lines have trailing whitespace.

50

Looking at this test we don't really need to worry about the differences between the codegen specific to Aix vs Linux. The important part of the test is showing that we don't shrink-wrap into a basic-block that is executed significantly more then the entry and exit blocks (IIUC). We can use regex to remove the difference between the labels, and skip some of the instructions which aren't critical to the test, and end up using all the same checks for Linux and 64-bit AIX, then a couple different checks for 32-bit codegen.

; CHECK-LABEL: {{[\.]?}}shrinkwrapme:
; CHECK:       # %bb.0:
; CHECK-NEXT:    cmpwi
; Prolog code
; CHECK64:         std
; CHECK64:         std
; CHECK64:         std
; CHECK64:         std
; CHECK64:         std
; CHECK64:         std
; CHECK64:         std
; CHECK64:         std
; CHECK64:         std
; CHECK64:         std
; CHECK64:         std
; CHECK64:         std
; CHECK64:         std
; CHECK64:         std
; CHECK64:         std
; CHECK64:         std
; CHECK64:         std
; CHECK64:         std

; CHECK32:         stw
; CHECK32:         stw
; CHECK32:         stw
; CHECK32:         stw
; CHECK32:         stw
; CHECK32:         stw
; CHECK32:         stw
; CHECK32:         stw
; CHECK32:         stw
; CHECK32:         stw
; CHECK32:         stw
; CHECK32:         stw
; CHECK32:         stw
; CHECK32:         stw
; CHECK32:         stw
; CHECK32:         stw
; CHECK32:         stw
; CHECK32:         stw

; CHECK:         blt 0, {{.*}}BB0_3
; CHECK:       # %bb.1:
; CHECK:         li
; CHECK:       {{.*}}BB0_2:
; CHECK:         add
; CHECK:         bdnz {{.*}}BB0_2
; CHECK-NEXT:    b {{.*}}BB0_4
; CHECK:       {{.*}}BB0_3:
; CHECK-NEXT:    li
; CHECK:       {{.*}}BB0_4:

; Epilog code
; CHECK64-DAG:         extsw
; CHECK64-DAG:         ld
; CHECK64-DAG:         ld
; CHECK64-DAG:         ld
; CHECK64-DAG:         ld
; CHECK64-DAG:         ld
; CHECK64-DAG:         ld
; CHECK64-DAG:         ld
; CHECK64-DAG:         ld
; CHECK64-DAG:         ld
; CHECK64-DAG:         ld
; CHECK64-DAG:         ld
; CHECK64-DAG:         ld
; CHECK64-DAG:         ld
; CHECK64-DAG:         ld
; CHECK64-DAG:         ld
; CHECK64-DAG:         ld
; CHECK64-DAG:         ld
; CHECK64-DAG:         ld

; CHECK32:         lwz
; CHECK32:         lwz
; CHECK32:         lwz
; CHECK32:         lwz
; CHECK32:         lwz
; CHECK32:         lwz
; CHECK32:         lwz
; CHECK32:         lwz
; CHECK32:         lwz
; CHECK32:         lwz
; CHECK32:         lwz
; CHECK32:         lwz
; CHECK32:         lwz
; CHECK32:         lwz
; CHECK32:         lwz
; CHECK32:         lwz
; CHECK32:         lwz
; CHECK32:         lwz

; CHECK:         blr

We can even skip the extsw instruction in the 64-bit prologue and simplify by removing the 'DAG' checks.

jsji added inline comments.Wed, Feb 10, 8:41 AM
llvm/test/CodeGen/PowerPC/shrink-wrap.ll
31–32

You can use <PREFIX>-COUNT-<num> to match multiple lines with the same pattern over and over again.

sfertile added inline comments.Wed, Feb 10, 9:58 AM
llvm/test/CodeGen/PowerPC/shrink-wrap.ll
2

I would stick with power9 for the cpu on the AIX runsteps also, just to keep consilient with the existing runstep.

llvm/test/CodeGen/PowerPC/shrink-wrap.mir
3

ditto on using same cpu as existing runstep.

sidbav updated this revision to Diff 322837.Wed, Feb 10, 3:10 PM

Address Comments, remove redundant checks.

sidbav marked 6 inline comments as done.Wed, Feb 10, 3:10 PM

I'll need a bit more time to finish going through ppc-shrink-wrapping.ll but the patch is looking really good. Thanks for the updates.

llvm/test/CodeGen/PowerPC/ppc64-sibcall-shrinkwrap.ll
5

This test is about shrink-wrapping in the presence of sibling calls, but for AIX we won't emit sib calls or tail calls.
I suggest having a single run step for AIX which uses -tailcallopt -disable-ppc-sco=false --enable-shrink-wrap=true options and checks for the error we emit in the backend. Then when we address tail-calling it will remind us to update this test.

llvm/test/CodeGen/PowerPC/shrink-wrap.ll
31–32

I didn't know we could do this, it really helps the readability of this test.

sidbav updated this revision to Diff 323326.Fri, Feb 12, 7:17 AM

Update ppc64-sibcall-shrinkwrap.ll to currently fail since tail calling is not implemented yet.

sidbav marked 2 inline comments as done.Fri, Feb 12, 7:18 AM
sfertile added inline comments.Fri, Feb 12, 8:37 AM
llvm/test/CodeGen/PowerPC/ppc-shrink-wrapping.ll
28

This should stay a `CHECK' lable, we expect this instruction on all platforms.

87

This should stay a `CHECK' lable, we expect this instruction on all platforms.

218

This should stay a `CHECK' lable, we expect this instruction on all platforms.

219–231

We expect the same instructions on both 64-bit targets, we just schedule them slightly differently between Linux and Aix. we should be able to common them up by checking more instructions and using check dag. Something like:

; CHECK: mflr {{[0-9]+}}

; DISABLE64-DAG: std {{[0-9]+}}
; DISABLE64-DAG: std {{[0-9]+}}
; DISABLE64-DAG: std {{[0-9]+}}
; DISABLE64-DAG: stdu 1,
; DISABLE64-DAG: cmplwi 3, 0

; DISABLE32-DAG: stw {{[0-9]+}}
; DISABLE32-DAG: stw {{[0-9]+}}
; DISABLE32-DAG: stw {{[0-9]+}}
; DISABLE32-DAG: stwu 1,
; DISABLE32-DAG: cmplwi 3, 0

; DISABLE-NEXT: beq 0, {{.*}}[[ELSE_LABEL:BB[0-9_]+]]
416–418

This check line and the next should be common for both 64-bit targets.

438–439

This check line and the next should be common for both 64-bit targets.

548

CHECK-LINUX-NEXT -> CHECK-NEXT

551

ENABLE-LINUX -> ENABLE

sfertile added inline comments.Fri, Feb 12, 9:49 AM
llvm/test/CodeGen/PowerPC/ppc-shrink-wrapping.ll
307

These 2 checks should be combined:
ENABLE-NEXT: beq 0, {{.*}}[[ELSE_LABEL:BB[0-9_]+]]

313–325

Similarly here we should not need to differentiate based on target OS. Something like:

; DISABLE64-DAG: std {{[0-9]+}}
; DISABLE64-DAG: std {{[0-9]+}}
; DISABLE64-DAG: std {{[0-9]+}}
; DISABLE64-DAG: stdu 1,
; DISABLE64-DAG: cmplwi 3, 0

; DISABLE32-DAG: stw {{[0-9]+}}
; DISABLE32-DAG: stw {{[0-9]+}}
; DISABLE32-DAG: stw {{[0-9]+}}
; DISABLE32-DAG: stwu 1,
; DISABLE32-DAG: cmplwi 3, 0

; DISABLE-NEXT: beq 0, {{.*}}[[ELSE_LABEL:BB[0-9_]+]]
sfertile added inline comments.Fri, Feb 12, 10:23 AM
llvm/test/CodeGen/PowerPC/ppc-shrink-wrapping.ll
418

Lets change the DISABLE-32AIX label to DIABLE32 to highlight the differences are due to 32-bit vs 64-bit codegen and not Linux vs AIX.

508

DISABLE-LINUX --> DISABLE

sidbav updated this revision to Diff 323464.Fri, Feb 12, 1:27 PM

Updates based on feedback.

sidbav marked 12 inline comments as done.Fri, Feb 12, 1:27 PM
sfertile added inline comments.Tue, Feb 16, 7:44 AM
llvm/test/CodeGen/PowerPC/ppc-shrink-wrapping.ll
311

CHECK-LINUX -> CHECK

397

Real minor nit: delete this line.

433

CHECK-LINUX -> CHECK-DAG

440–441

Real minor nit: delete this line.

633–634

The original test is checking that we generate a bclr instruction in the functions body (ie in-between func_begin and func_end). The Label checks ensures we are inside the expected function though so we can simplify this to:

; CHECK-LABEL: {{.*}}infiniteloop3
; CHECK: bclr
sidbav updated this revision to Diff 324019.Tue, Feb 16, 8:43 AM

Address Comments

sidbav marked 5 inline comments as done.Tue, Feb 16, 8:44 AM
sfertile retitled this revision from [ShrinkWrap] Enable Shrinkwrapping on 32 and 64 bit AIX, and 64 bit ELF to [PowerPC][AIX] Enable Shrinkwrapping on 32 and 64 bit AIX..Wed, Feb 17, 6:17 AM
sfertile accepted this revision as: sfertile.Wed, Feb 17, 6:21 AM

LGTM.

llvm/test/CodeGen/PowerPC/ppc64-sibcall-shrinkwrap.ll
6

Real minor nit: when commenting in a lit test we use ;; to make it standout more.

This revision is now accepted and ready to land.Wed, Feb 17, 6:21 AM
sidbav updated this revision to Diff 324294.Wed, Feb 17, 6:52 AM

Address comments

sidbav marked an inline comment as done.Wed, Feb 17, 6:52 AM
This revision was automatically updated to reflect the committed changes.