This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Add support for intrinsics dcbfps and dcbstps in P10.
ClosedPublic

Authored by Esme on Nov 12 2020, 12:23 AM.

Details

Summary

This patch added support for the intrinsics llvm.ppc.dcbfps and llvm.ppc.dcbstps.
dcbfps and dcbstps are actually extended mnemonics of dcbf.

dcbfps RA,RB ---> dcbf RA,RB,4
dcbstps RA,RB ---> dcbf RA,RB,6

Diff Detail

Event Timeline

Esme created this revision.Nov 12 2020, 12:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 12 2020, 12:23 AM
Esme requested review of this revision.Nov 12 2020, 12:23 AM
Esme added a reviewer: Restricted Project.
amyk accepted this revision as: amyk.Nov 12 2020, 3:29 PM
amyk added a subscriber: amyk.

This LGTM.

llvm/test/CodeGen/PowerPC/dcbf-p10.ll
2

I think it would be good to add a BE line when testing new intrinsics.

This revision is now accepted and ready to land.Nov 12 2020, 3:29 PM
Esme updated this revision to Diff 305022.Nov 12 2020, 9:09 PM

Added a BE line in the test.

steven.zhang added inline comments.Nov 12 2020, 10:42 PM
llvm/include/llvm/IR/IntrinsicsPowerPC.td
38

I didn't go into the detail of the definition of the intrinsic. But could you please double confirm if we need to set IntrWriteMem, IntrArgMemOnly as it seems that dbcf stuff is trying to write memory.

Esme added inline comments.Nov 16 2020, 7:23 PM
llvm/include/llvm/IR/IntrinsicsPowerPC.td
38

Thanks for your comment. The dcbf_ instructions perform the writes to main storage instead of memory. So there is no need to set IntrWriteMem, IntrArgMemOnly. Please correct me if I am wrong.

Esme updated this revision to Diff 307235.Nov 23 2020, 8:03 PM

According to ISA,

DCBF (Data Cache Block Flush) is treated as a Load, except that reference and change recording need not be done.

it is appropriate to set IntrReadMem, IntrArgMemOnly for these dcbf_ instructions.

steven.zhang accepted this revision.Nov 23 2020, 10:58 PM

LGTM now.

Esme added a comment.Nov 24 2020, 10:46 PM

@steven.zhang I still have some doubts about the properties setting. According to the behavior described by ISA, it also includes the write operation?

...those locations are written to main storage and additional locations in the block may be written to main storage...

I think it would be a safer way to only set IntrArgMemOnly.

This revision was landed with ongoing or failed builds.Dec 6 2020, 9:20 PM
This revision was automatically updated to reflect the committed changes.