Page MenuHomePhabricator

[PowerPC] VSX register support for inline assembly
ClosedPublic

Authored by ZhangKang on Mon, Dec 3, 2:26 AM.

Details

Summary

The patch is to add the VSX register support for inline assembly.

Before this patch, below case will get an error.

$ cat test.c
#include <altivec.h>
unsigned int a=0;
int main () {
  unsigned int *dbell=&a;
  int d;
 __asm__ __volatile__
                ("lxvw4x %%vs32, 0, %2\n\t"
                 "stxvw4x %%vs32, 0, %1"
                 : "=m"(*(volatile unsigned int*)(dbell)) 
                 : "r" (dbell),
                    "r" (&d)
                 : "vs32");  

  return 0;
}

Below is the error information.

$ clang -c test.c
t1.c:12:20: error: unknown register name 'vs32' in asm
                 : "vs32");  
                   ^
1 error generated.

After this patch, we can get inline assembly we expected without error.

Diff Detail

Repository
rC Clang

Event Timeline

ZhangKang created this revision.Mon, Dec 3, 2:26 AM
jsji requested changes to this revision.Mon, Dec 3, 10:05 AM

I think we should override getGCCAddlRegNames to map vs* to reg numbers ,
this can avoid unnecessary renaming from vs32 to v0 in clobber list.

call void asm sideeffect "lxvw4x %vs32, 0, $2\0A\09stxvw4x %vs32, 0, $1", "=*m,r,r,~{vs32}"(i32* %0, i32* %1, i32* %d) #1, !srcloc !3

This revision now requires changes to proceed.Mon, Dec 3, 10:05 AM
ZhangKang updated this revision to Diff 176560.Tue, Dec 4, 1:37 AM

The old patch will rename from vs32 to` v0` in clobber list, that‘s is not very reasonable.
The new patch will override getGCCAddlRegNames to map vs* to reg numbers, this can avoid unnecessary renaming from vs32 to` v0` in clobber list.

@jsji , I have uploaded a new patch to avoid renaming from vs32 to` v0` in clobber list.

jsji requested changes to this revision.Tue, Dec 4, 8:36 AM

RegNum are wrong!

clang/lib/Basic/Targets/PPC.cpp
416 ↗(On Diff #176560)

RegNum is wrong here!
"vs0" should be mapped to RegNum of "f0", which is 33.
"vs32" should be mapped to RegNum of "v0", which is 78.

This revision now requires changes to proceed.Tue, Dec 4, 8:36 AM
ZhangKang updated this revision to Diff 176923.Wed, Dec 5, 8:38 PM
ZhangKang marked 2 inline comments as done.

Fix the VSX register index mappping of the array GCCAddlRegNames.

@jsji I have updated a new patch for fix the error VSX register index mapping of the old patch.

clang/lib/Basic/Targets/PPC.cpp
416 ↗(On Diff #176560)

I will learn it and check it, thanks for your point out my error.

416 ↗(On Diff #176560)

Here, the value of AddlRegName is responding to the gcc macro ADDITIONAL_REGISTER_NAMES. In the array ADDITIONAL_REGISTER_NAMES, vs0~vs31 should be mapped to RegNum of f0~f31, which is 32~63. And vs32~vs63 should be mapped to RegNum of v0~v31, which is 77~108.

ZhangKang marked an inline comment as done.Wed, Dec 5, 10:26 PM
ZhangKang added inline comments.
clang/lib/Basic/Targets/PPC.cpp
416 ↗(On Diff #176560)

Below is the ADDITIONAL_REGISTER_NAMES GCC manual said:

Macro: ADDITIONAL_REGISTER_NAMES

    If defined, a C initializer for an array of structures containing a name and a register number. This macro defines additional names for hard registers, thus allowing the asm option in declarations to refer to registers using alternate names.
ZhangKang marked an inline comment as done.Wed, Dec 5, 10:26 PM
jsji accepted this revision.Thu, Dec 6, 7:06 AM

LGTM. Thanks for fixing.

clang/lib/Basic/Targets/PPC.cpp
416 ↗(On Diff #176560)

Yes, should be 32-63, 77-108. I forgot to use 0 as index start.

This revision is now accepted and ready to land.Thu, Dec 6, 7:06 AM
nemanjai added inline comments.Thu, Dec 6, 7:06 AM
clang/lib/Basic/Targets/PPC.cpp
416 ↗(On Diff #176560)

Is it possible to just not use magic numbers here at all? Do these correspond to something that is available in .td files? Is any of this possibly subject to change? Or if it is defined in some predefined macro/system header, etc. can we use the values from there?

ZhangKang marked an inline comment as done.Thu, Dec 6, 7:28 AM
jsji added inline comments.Thu, Dec 6, 7:32 AM
clang/lib/Basic/Targets/PPC.cpp
416 ↗(On Diff #176560)

This is fixed and defined in ABI, see ELFABIv2 "Table 2.26. Mappings of Common Registers".
AFAIK all ABIs use the same encoding.
Unfortunately, I don't think we have any td files or macros define these values.

@ZhangKang Please add a comment before this array,
so that it is easier to find doc about how these RegNum encoding are defined.

ZhangKang added inline comments.Thu, Dec 6, 8:03 AM
clang/lib/Basic/Targets/PPC.cpp
416 ↗(On Diff #176560)

If we don't use these numbers here, we can modify the array GCCRegAliases[] to add the map like {{"vs32"}, "v0"}, this method is my first patch for this pull-request. The first patch is in: https://reviews.llvm.org/D55192?id=176326
The first patch we use vs32 in inline-asm clobber list, we can see v0 in the IR clobber list not vs32. The lastest patch use GCCAddlRegNames, so we can see vs32 here.

I haven't found the correspond td files for these numbers. But these number of GCC ADDITIONAL_REGISTER_NAMES is same as DWARF Register Number Mapping. I think the GCC ADDITIONAL_REGISTER_NAMES is the PowerPC DWARF Register Number Mapping. So I think the numbes will not change for specify architecture, we can use these numbers.

You can found the DWARF Register Number Mapping in this website: http://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi.html#DW-REG . Also the DwarfEHRegSizeTable for PowerPC in clang is in: llvm/llvm/clang//lib/CodeGen/TargetInfo.cpp:4848~4893, this is the definitoin of PPC64_initDwarfEHRegSizeTable.

And I have checked the value of GCCAddlRegNames for SystemZ clang, its number also same as the SystemZ DWARF Register Number Mapping.

ZhangKang updated this revision to Diff 176990.Thu, Dec 6, 9:06 AM

Add the comment for the array AddlRegName to tell about how these RegNum encoding are defined.

ZhangKang marked an inline comment as done.Thu, Dec 6, 9:07 AM

I suppose since it is in the ABI, it is fine to leave the magic numbers there. And thanks for adding the comment. However, please verify that this mapping applies for ELF v1 as well and if it doesn't please add an appropriate guard.

It seems that, there are more registers in ABI about the additional register than we present in this patch. It would be great if we would list all of them here or add some comments to indicate this at least.

ZhangKang updated this revision to Diff 177132.Fri, Dec 7, 12:06 AM

Because the ELFv1 is different with ELFv2, so I add the guard before use GCCAddlRegNames.

ZhangKang marked 2 inline comments as done.Fri, Dec 7, 12:07 AM
This revision was automatically updated to reflect the committed changes.