This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Fix popcntb XL Compat Builtin for 32bit
ClosedPublic

Authored by quinnp on Jul 2 2021, 10:05 AM.

Details

Summary

This patch implements the __popcntb XL compatibility builtin for 32bit in the frontend and backend. This patch also updates tests for __popcntb and other XL Compat sync related builtins.

Diff Detail

Event Timeline

quinnp created this revision.Jul 2 2021, 10:05 AM
quinnp requested review of this revision.Jul 2 2021, 10:05 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 2 2021, 10:05 AM
quinnp added reviewers: Restricted Project, stefanp, nemanjai.Jul 2 2021, 10:12 AM
quinnp edited the summary of this revision. (Show Details)Jul 2 2021, 10:38 AM
amyk added a subscriber: amyk.Jul 5 2021, 6:36 AM
amyk added inline comments.
llvm/lib/Target/PowerPC/PPCInstr64Bit.td
912–915

Can we add a comment on why this is needed?

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

You can use update_llc_test_checks.py to update these.
Also nit, but might be nice to add -ppc-asm-full-reg-names.

NeHuang added a subscriber: NeHuang.Jul 5 2021, 9:16 AM
NeHuang added inline comments.
clang/test/CodeGen/builtins-ppc-xlcompat-sync.c
241

From the document, __icbt only valid when -qarch is set to target pwr8 or higher processors. It looks like target cpu sema checking and error case are missing. If you are working on another patch to fix it, please put fixme comments for the __icbt implementation and test case in this patch.

469

same as above.

llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-sync-64.ll
84

same here, pwr8 (or later processors) only

quinnp updated this revision to Diff 356719.Jul 6 2021, 7:38 AM

Addressing review comments.

nemanjai accepted this revision.Jul 12 2021, 7:41 PM

LGTM.

This revision is now accepted and ready to land.Jul 12 2021, 7:41 PM
amyk accepted this revision.Jul 13 2021, 7:43 AM

Minor indentation comments but LGTM.

clang/test/CodeGen/builtins-ppc-xlcompat-sync.c
3
llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-msync.ll
3

Minor indentation nit.

llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-sync-32.ll
4

Minor indentation nit.

llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-sync-64.ll
4

Minor indentation nit.

efriedma added inline comments.
llvm/lib/Target/PowerPC/PPCInstr64Bit.td
913

I'm sort of confused by the instruction variations here... let me see if I'm understanding correctly:

  1. popcntb always produces a 64-bit result.
  2. The 32-bit variation of the intrinsic just throws away the high bits.
  3. We can't use the 64-bit instruction in "32-bit" mode because we marked the register class illegal in isel, and Feature64BitRegs has been marked "beta" for the last 15 years.
  4. Therefore, there are two versions of the instruction: the real instruction that produces a 64-bit result, and a fake version of the instruction we can use in 32-bit mode.
quinnp updated this revision to Diff 358609.Jul 14 2021, 7:59 AM
quinnp marked 9 inline comments as done.

Addressing review comment about indentation.

quinnp updated this revision to Diff 358637.Jul 14 2021, 9:21 AM

Removing a misleading comment.

quinnp updated this revision to Diff 358639.Jul 14 2021, 9:24 AM

Removing another comment.

nemanjai added inline comments.Jul 14 2021, 10:17 AM
llvm/lib/Target/PowerPC/PPCInstr64Bit.td
913

This is correct. We have two GPR register classes that model the same register file. The gprc registers pretend the high bits don't exist. The two versions of the instructions that have GPR operands are quite common in PPC.

There have historically been some operating systems on which context switches spill the full width GPRs even in 32-bit mode. Others spill only the low 32 bits of the GPRs. The addition of two different register classes and Feature64BitRegs likely had to do with that.

efriedma added inline comments.Jul 14 2021, 10:50 AM
llvm/lib/Target/PowerPC/PPCInstr64Bit.td
913

There have historically been some operating systems on which context switches

Oh, that makes sense. You can't really use the high bits of a 64-bit register if they can get randomly zeroed out from under you.

Are any commonly used operating systems today in the "spill only the low 32 bits" category?

nemanjai added inline comments.Jul 14 2021, 11:41 AM
llvm/lib/Target/PowerPC/PPCInstr64Bit.td
913

Unfortunately, 32-bit AIX is in this category. We are hoping to be able to change that so that once we no longer support the old versions, we have more freedom with code generation.

This revision was landed with ongoing or failed builds.Jul 15 2021, 11:20 AM
This revision was automatically updated to reflect the committed changes.
llvm/test/CodeGen/PowerPC/builtins-ppc-xlcompat-sync-64.ll