Download Raw Diff

Details

Reviewers

nemanjai
echristo
hfinkel
jsji
steven.zhang

Commits

rG9606d58a5fae: [PowerPC] VSX register support for inline assembly
rL348572: [PowerPC] VSX register support for inline assembly
rC348572: [PowerPC] VSX register support for inline assembly

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.Dec 3 2018, 2:26 AM

Herald added a subscriber: eraman. · View Herald TranscriptDec 3 2018, 2:26 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.Dec 3 2018, 10:05 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.

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.Dec 4 2018, 8:36 AM

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.Dec 5 2018, 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.Dec 5 2018, 10:26 PM

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.Dec 6 2018, 7:06 AM

nemanjai added inline comments.Dec 6 2018, 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.Dec 6 2018, 7:28 AM

jsji added inline comments.Dec 6 2018, 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.Dec 6 2018, 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`.

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

ZhangKang marked an inline comment as done.Dec 6 2018, 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.

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

ZhangKang marked 2 inline comments as done.Dec 7 2018, 12:07 AM

Closed by commit rC348572: [PowerPC] VSX register support for inline assembly (authored by ZhangKang). · Explain WhyDec 7 2018, 1:02 AM

This revision was automatically updated to reflect the committed changes.

Herald added a subscriber: cfe-commits. · View Herald TranscriptDec 7 2018, 1:02 AM

This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] VSX register support for inline assembly
ClosedPublic

Details

Diff Detail

Event Timeline

Revision Contents

Diff 177141

lib/Basic/Targets/PPC.h

lib/Basic/Targets/PPC.cpp

test/CodeGen/inline-asm-matching-ppc-vsx.c

This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] VSX register support for inline assemblyClosedPublic

Details

Diff Detail

Event Timeline

Revision Contents

Diff 177141

lib/Basic/Targets/PPC.h

lib/Basic/Targets/PPC.cpp

test/CodeGen/inline-asm-matching-ppc-vsx.c

[PowerPC] VSX register support for inline assembly
ClosedPublic