Page MenuHomePhabricator

[AIX][XCOFF] emit vector info of traceback table.
Needs ReviewPublic

Authored by DiggerLin on Dec 21 2020, 12:41 PM.

Details

Summary

emit vector info of traceback table.

Diff Detail

Unit TestsFailed

TimeTest
50 msx64 debian > LLVM.CodeGen/PowerPC::aix-emit-tracebacktable-vectorinfo_hasvarg.ll
Script: -- : 'RUN: at line 7'; not --crash /mnt/disks/ssd0/agent/llvm-project/build/bin/llc -verify-machineinstrs -mtriple powerpc-ibm-aix-xcoff -mcpu=pwr7 -mattr=+altivec -vec-extabi -xcoff-traceback-table=true 2>&1 < /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/PowerPC/aix-emit-tracebacktable-vectorinfo_hasvarg.ll | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck --allow-unused-prefixes=false --check-prefixes=CHECK-ASM,COMMON /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/PowerPC/aix-emit-tracebacktable-vectorinfo_hasvarg.ll
70 msx64 windows > LLVM.CodeGen/PowerPC::aix-emit-tracebacktable-vectorinfo_hasvarg.ll
Script: -- : 'RUN: at line 7'; not --crash c:\ws\w64\llvm-project\premerge-checks\build\bin\llc.exe -verify-machineinstrs -mtriple powerpc-ibm-aix-xcoff -mcpu=pwr7 -mattr=+altivec -vec-extabi -xcoff-traceback-table=true 2>&1 < C:\ws\w64\llvm-project\premerge-checks\llvm\test\CodeGen\PowerPC\aix-emit-tracebacktable-vectorinfo_hasvarg.ll | c:\ws\w64\llvm-project\premerge-checks\build\bin\filecheck.exe --allow-unused-prefixes=false --check-prefixes=CHECK-ASM,COMMON C:\ws\w64\llvm-project\premerge-checks\llvm\test\CodeGen\PowerPC\aix-emit-tracebacktable-vectorinfo_hasvarg.ll

Event Timeline

DiggerLin created this revision.Dec 21 2020, 12:41 PM
DiggerLin requested review of this revision.Dec 21 2020, 12:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 21 2020, 12:41 PM
jasonliu added inline comments.Dec 22 2020, 12:04 PM
llvm/lib/BinaryFormat/XCOFF.cpp
142

Something is wrong if Value != 0u, but I think it's normal when we have more parameters than the uint32 type could encode. And in those situation, as Hubert suggested, we could print ... to represent any values that we could not decode. Please have a test case for this case as well.
Same for parseParmsTypeWithVecInfo function.

llvm/lib/Object/XCOFFObjectFile.cpp
852

This Error as an parameter in the constructor is too easy to get ignored. If we need to do this, we would do the whole set which includes making this constructor private, and have a Create function to return an error or the object, which is described here: https://llvm.org/docs/ProgrammersManual.html#fallible-constructors

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
2054

This .get() won't work if you are compiling in a non-assert mode.

llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp
70

Please fix all clang-tidy warnings.

70

Instead of traversing this array for 3 times (getFixedParmsNum, getFloatingPointParmsNum, getVectorParmsNum), why not just still increment the corresponding data member in appendParameterType()?

73

You could pull this out of the every case, and put it just before the switch statement. As you are doing it for every case.

76

You could pull this out of the every case, and put it just after the switch statement. As you are doing it for every case.

97

An assert here would be good.

DiggerLin marked 8 inline comments as done.Jan 4 2021, 1:18 PM
DiggerLin added inline comments.
llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp
70

I think we only use here for getFloatingPointParmsNum, getVectorParmsNum once.
I can increment corresponding data member , but it need three new data member here.

73

it can not pull out, for there are other parameter type (for example FixedType), we do not need deal with in the function.

97

I can not add assert here for there are other parameter type (for example FixedType).

DiggerLin updated this revision to Diff 314448.Jan 4 2021, 1:45 PM
DiggerLin marked 3 inline comments as done.
jasonliu added inline comments.Jan 6 2021, 2:40 PM
llvm/include/llvm/Object/XCOFFObjectFile.h
398–399

Let's remove this "friend" so that no one could call the constructor directly.

llvm/lib/BinaryFormat/XCOFF.cpp
229–230

minor style nit.

255

default assert here?

llvm/lib/Object/XCOFFObjectFile.cpp
923–924
960

I thought we only need to do this ErrorAsOutParameter once at the beginning of the function. Why do we need to do it 3 times at here and 981 and 992?

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1968

Only checking error in assert won't work in non-assertion build.

2017

I think we are only supposed to set this bit when the special register VRSAVE is saved on stack.

2030

There is also a VSCR register, do we need to check that? Or are we inferring checking V0 to V31 would be enough to find out if any vmx instruction is used? If so, a comment would be desired.

llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp
69

What if we don't have parameters that are vectors, but we use vectors inside of the function? I thought we should set HasVectorInfo in those situation as well.

70

It's possible these functions got called later for other purposes and we don't know.
If we need new data members then we could just add it, I don't think the memory consumption of three unsigned types are too bad (comparing to three suboptimal traverse).

75
183

Address the Lint comment.

llvm/test/CodeGen/PowerPC/aix-emit-tracebacktable-vectorinfo.ll
57

Is it possible to have tests for these fields?

DiggerLin marked 11 inline comments as done.Jan 11 2021, 9:02 AM
DiggerLin added inline comments.
llvm/include/llvm/Object/XCOFFObjectFile.h
398–399

thanks

llvm/lib/BinaryFormat/XCOFF.cpp
255

there 4 cases for 2 bits, I do not think we need default here.

llvm/lib/Object/XCOFFObjectFile.cpp
923–924

thanks

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1968

thanks

2017

Hubert point out that XL sets the bit (at least when there are other vector registers saved) , maybe we need double confirm with hubert @hubert.reinterpretcast

2030

I do not think we need to check the VSCR. for "Special instructions
(mfvscr and mtvscr) are provided to move the VSCR from and to a vector register"
when operated on the vscr, there must be Vector register , we can check register PPC::V0 ~PPC::V31 .

llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp
69

thanks for point out.

llvm/test/CodeGen/PowerPC/aix-emit-tracebacktable-vectorinfo.ll
57

I can not add a test case for it. There is error as " error in backend: variadic arguments for vector types are unimplemented for AIX"

DiggerLin marked 8 inline comments as done.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
2017

You're both right. XL sets the bit (I don't think it was supposed to). The funny thing is that setting the bit prevents a possible misinterpretation of the traceback table by the XL C++ runtime because of a confluence of errors.

jasonliu added inline comments.Jan 11 2021, 4:23 PM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1870

I have concerns about writing our own version of this function.
I think it's important to understand why the MRI version doesn't work. Is it a bug or work as intended? If writing our own version is really necessary, I would avoid naming them the same as it's going to cause confusion for people on when to use which version.

2016
llvm/test/CodeGen/PowerPC/aix-emit-tracebacktable-vectorinfo.ll
57

Sure. I saw you add a test case to test the error for HasVarArgs, which is good.
But what about NumOfVRsSaved = 0, -IsVRSavedOnStack? I don't think that get affected by the error.

llvm/test/CodeGen/PowerPC/aix-emit-tracebacktable-vectorinfo_hasvarg.ll
6

I don't think we want to use include in the lit test. You could write the declaration yourself. And you probably don't even need to call vec_abs if you just want to check if HasVarArgs bit is set?

DiggerLin marked 2 inline comments as done.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
2008

To coordinate with D96351, the number of vector registers saved should be zero in the default ABI.