Page MenuHomePhabricator

[llvm-exegesis][PowerPC] Add more register classes
ClosedPublic

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

Details

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.

jsji added a comment.Nov 4 2020, 7:12 AM

Ping.. Any further comments?

jsji added a comment.Nov 24 2020, 6:06 AM

Ping.. Any further comments?

qiucf added a comment.Nov 30 2020, 8:36 AM

Thanks for the patch! I think code added is quite clear since most of them already exist as framework in implementation of other platforms. We can continue to support more about exegesis on PowerPC in later patches.

Besides, I found llvm-exegesis -mode=latency -opcode-name=XSTDIVDP complains 'illegal instruction' on ppc64le-pwr9, which looks strange. Is that because we did not model CR well in this tool?

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

So, we will mark OPERAND_REGISTER in future patches?

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

I applied this patch and tested XSDIVDP, still got 2 lines of warning setRegTo is not implemented, results will be unreliable (before this patch, that is six). Is that from RM and can be ignored?

jsji added a comment.Nov 30 2020, 9:20 AM

Thanks @qiucf for looking into this.

Besides, I found llvm-exegesis -mode=latency -opcode-name=XSTDIVDP complains 'illegal instruction' on ppc64le-pwr9, which looks strange. Is that because we did not model CR well in this tool?

It depends. We will generate *random* instructions to form the pattern, so you should try to dump the object to see what is causing illegal instruction.

But on the other hand, yes, as I mentioned , this is an incremental patch, it is far from complete.
So some test may still invalid and causing failures.

We will work to fix them in following patches.

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

We may, but not necessary, it depends on whether we want to update OperandType for all td files or just update when it matters.

This patch only update for imm as it is required and used in llvm-exegesis.

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

Yes, you are right. This patch implemented *more* RC, but not a complete one. We will add more RC in later patch.

steven.zhang accepted this revision.Dec 1 2020, 6:44 PM

LGTM, but as mentioned, we need some follow up to support more instructions. Please hold on for several days to see if @nemanjai has more comments.

This revision is now accepted and ready to land.Dec 1 2020, 6:44 PM
This revision was landed with ongoing or failed builds.Dec 4 2020, 7:02 AM
This revision was automatically updated to reflect the committed changes.