This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Store, load, move from and to registers related builtins
ClosedPublic

Authored by Conanap on Jul 13 2021, 3:43 PM.

Details

Summary

This patch implements store, load, move from and to registers related
builtins. The patch aims to provide feature parady with xlC on AIX.

Diff Detail

Event Timeline

Conanap created this revision.Jul 13 2021, 3:43 PM
Conanap requested review of this revision.Jul 13 2021, 3:43 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 13 2021, 3:43 PM
Conanap edited the summary of this revision. (Show Details)Jul 13 2021, 3:50 PM
Conanap added reviewers: Restricted Project, nemanjai.
lei added a subscriber: lei.Jul 14 2021, 11:53 AM

please add sema checking for pwr8 builtins.

llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-LoadReserve-StoreCond.ll
80

remove all reference to attributes, local_unnamed_addr #[0..9] since it's not in the IR.

Conanap updated this revision to Diff 358738.Jul 14 2021, 1:58 PM
Conanap marked an inline comment as done.

Added more sema checking, test case update

lei added inline comments.Jul 15 2021, 6:26 AM
clang/lib/Sema/SemaChecking.cpp
3371

need tests for these.

llvm/include/llvm/IR/IntrinsicsPowerPC.td
1569

do these loads not need IntrReadMem?

nemanjai requested changes to this revision.Jul 15 2021, 10:08 AM

We have encountered an issue with lwarx/ldarx that required that they emit inline asm rather than an intrinsic. What makes lbarx/lharx different?

clang/lib/Sema/SemaChecking.cpp
3370

Why do these need the extdiv feature?

This revision now requires changes to proceed.Jul 15 2021, 10:08 AM
Conanap updated this revision to Diff 359066.Jul 15 2021, 11:29 AM

Added more tests, corrected sema checking and intrinsic flag

Conanap marked 3 inline comments as done.Jul 15 2021, 11:30 AM
lei added inline comments.Jul 15 2021, 12:02 PM
llvm/include/llvm/IR/IntrinsicsPowerPC.td
1569

nit: un-related line deletion

llvm/lib/Target/PowerPC/PPCInstrInfo.td
5449

EXTSH should not be needed and we should not be using xoaddr

llvm/test/CodeGen/builtins-ppc-xlcompat-move-tofrom-regs.ll
8

this is confusing... maybe this shouldjust be CHECK-32BIT

Conanap updated this revision to Diff 359097.Jul 15 2021, 12:54 PM
Conanap marked 2 inline comments as done.

Changed xoaddr, removed extws, changed check prefix

Conanap marked an inline comment as done.Jul 15 2021, 12:55 PM
Conanap updated this revision to Diff 359104.Jul 15 2021, 1:19 PM

Changed more xoaddr to ForceXForm

nemanjai requested changes to this revision.Jul 16 2021, 2:25 AM

Taking this off the review queue until lharx/lbarx are changed to emit inline asm in line with lwarx/ldarx.

This revision now requires changes to proceed.Jul 16 2021, 2:25 AM
Conanap updated this revision to Diff 359259.Jul 16 2021, 2:27 AM

Updated lharx and lbarx to inline asm implementation, implemented stfiw.

nemanjai requested changes to this revision.Jul 16 2021, 3:41 AM

This is getting close to approval. The newly added __stfiw needs to be fixed and some nits need to be addressed.

clang/lib/Sema/SemaChecking.cpp
3369

This is not correct. The instruction (non-VSX version) has existed since Power3. The VSX version was added in Power8. No changes to the instruction came in Power9 so I have no idea where the decision to add this check came from.

In fact, this would also blow up in the back end if you compiled with something like -mcpu=pwr9 -mno-altivec or -mcpu=pwr9 -mno-vsx.

llvm/include/llvm/IR/IntrinsicsPowerPC.td
1568

Nit: line too long.

1577

Nit: indentation is inconsistent here.

llvm/lib/Target/PowerPC/PPCInstrVSX.td
4072

This needs the non-VSX pattern as well.

llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-stfiw.ll
2

One of the run lines should be with -mattr=-vsx.

This revision now requires changes to proceed.Jul 16 2021, 3:41 AM
Conanap updated this revision to Diff 359322.Jul 16 2021, 7:48 AM
Conanap marked 5 inline comments as done.

Added non-vsx pattern for stfiw, extra testline for that pattern,
some nits

Conanap updated this revision to Diff 359380.Jul 16 2021, 10:24 AM

Updated a test case

nemanjai added inline comments.Jul 16 2021, 11:48 AM
llvm/lib/Target/PowerPC/PPCInstrVSX.td
4072

I just realized this is in the wrong place. STXSIWX was added in Power8 not in Power9.

llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-stfiw.ll
11

Don't make all of these pwr9. Make some of them pwr7 and some pwr8.

lei added inline comments.Jul 16 2021, 1:09 PM
clang/test/CodeGen/builtins-ppc-xlcompat-LoadReseve-StoreCond.c
15

where is the check for CHECK-NON-PWR8-ERR:?

36

CHECK-NON-PWR8-ERR: check?

clang/test/CodeGen/builtins-ppc-xlcompat-stfiw.c
3

why are we only testing this for pwr9 vs pwr7/8 similar to other tests added?

Conanap updated this revision to Diff 359433.Jul 16 2021, 1:41 PM
Conanap marked an inline comment as done.

Updated test cases

Conanap marked 4 inline comments as done.Jul 16 2021, 1:43 PM

Addressed comments

clang/test/CodeGen/builtins-ppc-xlcompat-LoadReseve-StoreCond.c
15

lwarx and stwcx are both available before power8, so the check is not needed.

Conanap updated this revision to Diff 359516.Jul 16 2021, 7:00 PM
Conanap marked an inline comment as done.

Moved pattern to a more appropriate place, updated test cases

nemanjai accepted this revision.Jul 18 2021, 2:56 PM

LGTM.

This revision is now accepted and ready to land.Jul 18 2021, 2:56 PM
Conanap updated this revision to Diff 359850.Jul 19 2021, 11:22 AM

Changed flags for intrinsic of dcbtt and dcbtstt

Conanap updated this revision to Diff 359945.Jul 19 2021, 3:44 PM

Fixed a typo

This revision was landed with ongoing or failed builds.Jul 20 2021, 1:46 PM
This revision was automatically updated to reflect the committed changes.

I'm aware of the getting target issue; the fix will be up soon.

issue should be fixed now; pushed with this: https://reviews.llvm.org/D106130#change-PZi4uueeCg9i
(I just had to move the test files into the PowerPC folder).

Will continue to monitor

@jroelofs committed f6769b663a0d4432b5e00e0c03904a5dfba7b077 to move the backend test cases from CodeGen -> CodeGen/PowerPC so they don't fail when the PPC backend isn't built.