Page MenuHomePhabricator

[AIX][XCOFF] emit vector info of traceback table.
ClosedPublic

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

Details

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jasonliu added inline comments.Jan 6 2021, 2:40 PM
llvm/include/llvm/Object/XCOFFObjectFile.h
540–541

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

llvm/lib/Object/XCOFFObjectFile.cpp
1015

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
2176

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.

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
540–541

thanks

llvm/lib/BinaryFormat/XCOFF.cpp
254

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

llvm/lib/Object/XCOFFObjectFile.cpp
978–979

thanks

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

thanks

2163

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

2176

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
2163

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
2015

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.

2162
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
2154

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

DiggerLin updated this revision to Diff 329973.Mar 11 2021, 8:11 AM

address comment.

DiggerLin updated this revision to Diff 349315.Jun 2 2021, 10:29 AM

rebased on the latest source code

jasonliu added inline comments.Jun 3 2021, 7:18 AM
llvm/include/llvm/CodeGen/MachineRegisterInfo.h
859

If this is necessary, could we add a test case for it? Right now, even setting the third parameter to always be false, I don't see any lit failing.

llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.h
126

Do you need to store this info?
For hasVectorParms() function, could you do return VectorParmsNum; instead? Or you could also get rid of that function and use getVectorParmsNum() != 0.

DiggerLin marked 2 inline comments as done.Jun 3 2021, 12:10 PM
DiggerLin added inline comments.
llvm/include/llvm/CodeGen/MachineRegisterInfo.h
859

I do not think we need to new test case ,
in the function MachineRegisterInfo::isPhysRegUsed , we change the

if (UsedPhysRegMask.test(PhysReg))

return true;

--->

if (!SkipTestUsedPhysRegMask && UsedPhysRegMask.test(PhysReg))

return true;

and the the default value of SkipTestUsedPhysRegMask is false, it means , all the behaviors not change.

the behaviors only changes when SkipTestUsedPhysRegMask =true;

and we have invoked
MRI.isPhysRegModified(Reg, false, true) in several place in the function PPCAIXAsmPrinter::emitTracebackTable.
I think we have test SkipTestUsedPhysRegMask =true too.

llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.h
126

yes, I can delete the variable .

DiggerLin updated this revision to Diff 349639.Jun 3 2021, 12:11 PM
DiggerLin marked 2 inline comments as done.

address comment

jasonliu added inline comments.Jun 4 2021, 8:35 AM
llvm/include/llvm/CodeGen/MachineRegisterInfo.h
859

Discussed offline, we need to add a test if the extra parameter is needed.

860–863

Please change accordingly for the isPhysRegModified above as well.

llvm/lib/BinaryFormat/XCOFF.cpp
156–157

nit

161

Nit: Remove else.

268

nit: Remove else

llvm/lib/CodeGen/MachineRegisterInfo.cpp
586

I think this parameter name is too verbose. Suggest:
SkipTestUsedPhysRegMask -> SkipRegMaskTest

llvm/lib/Object/XCOFFObjectFile.cpp
1023

nit

1037

Is llvm-objdump capable of decoding the traceback table? Do we want to have a test case using llvm-objdump? Or are we thinking to put it on another patch?

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

Could we add a comment saying that we would only treat instructions uses vector register as vector instructions?

2034

For the places that we use raw boolean value, could we have a comment beside to state what that value is for?
i.e.
if (MRI.isPhysRegUsed(Reg, /* SkipRegMaskTest */ true)) {

2185–2190
llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp
143

Could we use a switch statement here instead instead of ifs? Also report_fatal_error if the Type is something we don't know.

jasonliu added inline comments.Jun 4 2021, 9:18 AM
llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp
199–202

Could we combine the above two if statement?

jasonliu added inline comments.Jun 4 2021, 9:52 AM
llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.h
135

Does this store the parameter types when the parameter is on stack? Right now, it doesn't look like so. And this sounds like we are trying to store all paramters including the one on stack.
Could we add a test case for testing parameters on stack?

DiggerLin marked 17 inline comments as done.Jun 8 2021, 7:34 AM
DiggerLin added inline comments.
llvm/lib/Object/XCOFFObjectFile.cpp
1037

we have patch which can decode traceback table of object file. https://reviews.llvm.org/D89049

llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp
199–202

can not combine it, for Elt == FixedType && !hasVectorParms() , it only need one bit, so when Bits ==31, we can still put one bit of FixedType which do not have hasVectorParms().

llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.h
135

we do not store the parameter types when the parameter is on stack.

DiggerLin marked 3 inline comments as done.Jun 8 2021, 10:53 AM
DiggerLin added inline comments.
llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.h
135

in the llvm/test/CodeGen/PowerPC/aix-emit-tracebacktable.ll , there are some parameter are stored on stack.

DiggerLin updated this revision to Diff 350672.Jun 8 2021, 11:29 AM

address comment

jasonliu added inline comments.Jun 9 2021, 12:00 PM
llvm/include/llvm/CodeGen/MachineRegisterInfo.h
860–862
llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.h
135

Consider this test case:

int foo(int a, int b, int c, float d, int e, int f, int g, int h, int i,  float j, int k) {
  return k;
}

The print out I get is:

.vbyte  4, 0x10400000                   # Parameter type = i, i, i, f, i, i, i, i, f

However, I'm expecting this if it supports parameter saved on stack correctly:

  1. Parameter type = i, i, i, f, i, i, i, i, i, f, i

Notice the two i are missing around the last f.

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

Could we add a comment here for this padding as well?

DiggerLin marked 2 inline comments as done.Jun 10 2021, 8:38 AM
DiggerLin added inline comments.
llvm/include/llvm/CodeGen/MachineRegisterInfo.h
860–862

thanks

llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.h
135

we only care about the parameter which are passed in register for Parameter type.
there are only 8 GPR for parameter saved. i, i, i, f(occupied on a FPR and burned a GPR), i, i, i, i, already 8 GPR.

DiggerLin marked 3 inline comments as done.Jun 10 2021, 8:39 AM
DiggerLin added inline comments.
llvm/test/CodeGen/PowerPC/aix-emit-tracebacktable-vectorinfo.ll
105

thanks

DiggerLin updated this revision to Diff 351186.Jun 10 2021, 8:40 AM
DiggerLin marked an inline comment as done.

added a padding comment for the vector info

jasonliu added inline comments.Jun 10 2021, 11:26 AM
llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp
199–202

So what would not work for the suggestions I made?

jasonliu added inline comments.Jun 10 2021, 11:31 AM
llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.h
125–126
128–129
131–136
135

Could you modify the above comment: /// ParamTypes - Store all the paramters type.
to be /// ParamtersType - Store all the parameter's type that are saved on registers.

DiggerLin marked 5 inline comments as done.Jun 10 2021, 1:20 PM
DiggerLin added inline comments.
llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp
199–202

change as suggestion, thanks

DiggerLin updated this revision to Diff 351247.Jun 10 2021, 1:23 PM
DiggerLin marked an inline comment as done.
DiggerLin updated this revision to Diff 351475.Jun 11 2021, 9:30 AM

rebased code

jasonliu accepted this revision.Jun 11 2021, 10:24 AM

LGTM; Thanks!

This revision is now accepted and ready to land.Jun 11 2021, 10:24 AM