Page MenuHomePhabricator

[llvm-exegesis][PowerPC] Add more register classes
Needs ReviewPublic

Authored by jsji on Sep 21 2020, 1:50 PM.

Details

Reviewers
courbet
gchatelet
steven.zhang
qiucf
Group Reviewers
Restricted Project
Summary

This PR adds more register class support in PowerPC,
mark OperandType for imm and memory operands.

Also added more unit tests for SnippetGenerator.

Diff Detail

Event Timeline

jsji created this revision.Sep 21 2020, 1:50 PM
jsji requested review of this revision.Sep 21 2020, 1:50 PM
jsji edited the summary of this revision. (Show Details)Sep 21 2020, 1:51 PM

This looks good for the llvm-exegesis part. I'm not familiar enough with the PowerPC target to review the rest.

llvm/unittests/tools/llvm-exegesis/PowerPC/TestBase.h
12

I think you meant POWERPC :)

jsji added inline comments.Sep 23 2020, 6:08 AM
llvm/unittests/tools/llvm-exegesis/PowerPC/TestBase.h
12

Good catch! Thanks,yeah, typical copy and paste error.

jsji updated this revision to Diff 293717.Sep 23 2020, 6:38 AM

Fix typo in header.

I think the code being added here (and perhaps the existing code as well) requires deep expertise in both PowerPC ISA and Exegesis. But I think with more descriptive comments, we can make it consumable for those that only check one of those boxes. So most of my comments are requests to add more descriptive/explanatory comments.

llvm/lib/Target/PowerPC/PPCInstr64Bit.td
39

I assume you have tracked down other definitions for immediate nodes that don't have this set and there are no others?

llvm/tools/llvm-exegesis/lib/PowerPC/Target.cpp
78

If the instruction is a store, won't the value being stored be one of the operands?

82

Because the stack pointer is guaranteed to point to accessible memory? I still don't understand why we wouldn't materialize the offset into a register here.
It seems odd that for D-Forms, we will access memory Reg + Offset and for X-Forms, we will access memory Reg + <Stack>. Seems this is something that should be documented.

103

Two concerns with this:

  1. I assume you are using X11 as it is the last caller saved reg in the allocation order and therefor least likely to be used. Can you document this (along with why you think it is safe to use)? Also, can you give it a descriptive name?
  2. There does not appear to be any checking that the immediate fits into a 16-bit signed field. Are we guaranteed to get small immediates? Or perhaps we don't really care that the value we get here is the actual value in the register? Please add a comment.
109

This seems highly suspicious and should be documented if it is correct. For VR registers, we populate a single doubleword. But for VSR registers we populate both doublewords? Why? Also, is there a check elsewhere for P9Vector since you're using MTVSRDD which was added in ISA 3.0?

jsji added inline comments.Sep 24 2020, 11:54 AM
llvm/lib/Target/PowerPC/PPCInstr64Bit.td
39

Yes. At least as far as I can tell now.

llvm/tools/llvm-exegesis/lib/PowerPC/Target.cpp
78

No. Value operand is not included.
This is for MemoryOperands only, which is actually only the addressing operand in real llvm MI..

82

I will add comment. No, we don't really care about the address for now.

103

Sure.
Yeah, we don't really care the actual value in register.
This is to form a valid MI, and repeat it for certain times, to measure latency/throughput etc, we don't really care about the values at all.

109

As I mentioned above, we don't really care about the values in reg. We just want to generate a sequence of valid assembly..

And this will generate a code in a snippet , did not check cpu at all for now.

jsji updated this revision to Diff 294163.Sep 24 2020, 2:23 PM

Address comments.