Page MenuHomePhabricator

Sample code for porting MachinePipeliner to AArch64+SVE
Needs RevisionPublic

Authored by masakiarai on Jun 8 2018, 6:31 AM.

Details

Summary

This commit is for `[RFC] Porting MachinePipeliner to AArch64+SVE'.

Diff Detail

Event Timeline

masakiarai created this revision.Jun 8 2018, 6:31 AM
rengolin added a subscriber: rengolin.

Adding some reviewers + folks on the original review:

https://reviews.llvm.org/D16829

Note: this is work-in-progress proof-of-concept for the RFC:

http://lists.llvm.org/pipermail/llvm-dev/2018-June/123884.html

fhahn added a comment.Jun 8 2018, 8:00 AM

Interesting! There are a few formatting issues, could you format the diff with clang-format?

Remove irrelevant differences and fix the format.

fhahn added a comment.Jun 11 2018, 4:04 AM

Thanks for updating the patch. I think it would be slightly easier to review this patch if you could provide a brief high-level description of the modelling and some test cases.

lib/CodeGen/MachinePipeliner.cpp
1394

could use range based for loop here?

1397

We need to ignore debug info here I think.

lib/Target/AArch64/AArch64InstrInfo.cpp
5106

Does this implementation satisfy the interface? According to TargetInstrInfo::reduceLoopCount, it should generate code to reduce the loop iteration by one.

5114

Couldn't we stop after we found the SUBSXrr closest to the terminator?

Thank you for your detailed comment.
Your points make sense to me.

I should have clarified the intent of this patch.
I do not think that this patch will be accepted.
This is aimed to show that we can generate code without DFAPacketizer with as few changes as possible.
For that reason, the method interface and code keeps the original code as much as possible.

lib/Target/AArch64/AArch64InstrInfo.cpp
5106

Hexagon uses special loop instructions to count down the loop counter.
On the other hand, in most cases on AArch64 or x86_64, we will target loops that count up the loop counter.
Therefore, I think that it is appropriate to make the function name TargetInstrInfo::fixLoopCount' rather than TargetInstrInfo::reduceLoopCount'.
I thought it was inappropriate to rewrite code for Hexagon, so I did not change it.

5114

Yes, you are right.
I should break the loop when I found the first CompMI.
There is the same bug for X86InstrInfo::reduceLoopCount.

kparzysz added inline comments.
lib/Target/AArch64/AArch64InstrInfo.cpp
5106

You can change the Hexagon code if it makes it easier to adopt the pipeliner for other architectures.

lsaba added a subscriber: lsaba.Jun 13 2018, 1:43 AM
lsaba removed a subscriber: lsaba.
lsaba added a subscriber: lsaba.
fhahn added a comment.Jun 28 2018, 3:20 AM

Thank you for your detailed comment.
Your points make sense to me.

I should have clarified the intent of this patch.
I do not think that this patch will be accepted.
This is aimed to show that we can generate code without DFAPacketizer with as few changes as possible.
For that reason, the method interface and code keeps the original code as much as possible.

Ok thanks. Is there anything else you want feedback on at the moment?

No, at the moment there is nothing.
Since I think there was no objection to the extension of MachinePipeliner, I am currently creating a patch aimed for upstreaming.
Please review it.
Thank you very much.

fhahn requested changes to this revision.Jan 11 2019, 8:47 AM

Marking this as requiring changes, as the MachinePipeliner code is getting refactored a bit in D56084. Please let us know when you want us to review the patch again.

This revision now requires changes to proceed.Jan 11 2019, 8:47 AM