Page MenuHomePhabricator

[PowerPC] Fix issue with lowering byval parameters.
ClosedPublic

Authored by stefanp on Aug 26 2021, 5:49 PM.

Details

Summary

Lowering of byval parameters with sizes that are not represented by a single
store require multiple stores to properly address the correct size of the
parameter.

Sizes that cannot be done with a single store are 3 bytes, 5 bytes, 6 bytes,
7 bytes. It is not correct to simply perform an 8 byte store and for these
elements because then the store would be larger than the element and alias
analysis would assume that this is undefined behaivour and return NoAlias
for them.

This patch adds the correct stores so that the size of the store is not larger
than the size of the element.

Event Timeline

stefanp created this revision.Aug 26 2021, 5:49 PM
stefanp requested review of this revision.Aug 26 2021, 5:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 26 2021, 5:49 PM
stefanp updated this revision to Diff 369014.Aug 26 2021, 5:53 PM

Fixed formatting on a couple of lines.

stefanp added reviewers: NeHuang, amyk, Restricted Project.Aug 26 2021, 5:53 PM
amyk added inline comments.Sep 13 2021, 6:31 PM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
4220

Can we add documentation to this function?

4373

Question: I just wanted to check whether or not using Arg in cases 3/5/6/7 is expected, or if it is supposed to be frame index like in the code before?

4380

Might be a silly question, but since it looks like the code for ObjSize == [1|2|4] looks like it is still the same, does it make sense for the switch to be in the else case for 3,5,6,7?
Or is it a preference to just transform the whole thing as a switch (like what you've done in the patch)? I'm just wondering as it kind of looks like the code in the cases are fairly repetitive for some of the cases.

4386

nit: It looks like for cases 3 and 6, there are { } surrounding the case statements. It may be good to put them around 5 and 7 for consistency.

llvm/test/CodeGen/PowerPC/ppc64-byval-multi-store.ll
3

nit: Indenting (for here and other lines)

stefanp updated this revision to Diff 374320.Sep 22 2021, 12:17 PM

Added a few comments and fixed some formatting.

Thank you for looking at this Amy. I'm sorry it too me so long to get to it.
I've addressed some of your comments and added responses to others.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
4373

I think using Arg in this case is correct. We do want to make sure that we adjust the address in the Big Endian situation. I want to be able to treat the 3/5/6/7 case in a similar way to the way that we create the single load situations.

4380

It's just a style preference for me to add the switch statement to the whole thing. The code for sizes 1, 2, 4 has not changed and I've just replaced the:

EVT ObjType = (ObjSize == 1 ? MVT::i8 :
               (ObjSize == 2 ? MVT::i16 : MVT::i32));

with the switch statement.
I like this better because I have to check the size anyway to get the EVT so there is no point in splitting this part off into its own code. At least that is how I feel from a style perspective.

4386

Actually, I'm going to remove the { } from all of the cases. I needed them when I was doing development but at this point they are not required.

lei added inline comments.Sep 24 2021, 10:12 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
4371–4372

This case stmt basically does:

  1. generate truc store for i8|i16|i32
  2. push truc store to MemOps

I think it would be more clean to create a lambda that does both steps with MVT::i[8|16|32] as an arg and call it within the case stmts.

amyk added a comment.Sep 30 2021, 6:57 AM

Thank you for answering my questions! For now, I have some additional nit comments regarding the comments you added.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
4223
4240

Do you mean:

nemanjai requested changes to this revision.Sep 30 2021, 8:14 PM

Please try out the simpler alternative of letting the legalizer take care of the store size. If that alternative is not possible, please add comments to the patch explaining why it is not.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
4371–4372

This seems unnecessarily complicated. There is already code in the legalizer that does this, so we should be able to just emit the store that we need.

If for whatever reason we can't, I would much rather not grow this already huge function by all this code but fold this into the new function.

llvm/test/CodeGen/PowerPC/ppc64-byval-multi-store.ll
1

Please pre-commit to show only the differences in code-gen.

This revision now requires changes to proceed.Sep 30 2021, 8:14 PM
stefanp updated this revision to Diff 376601.Oct 1 2021, 12:18 PM

Thank you to advice from Lei and Nemnaja helping to me significantly simplify this patch.

nemanjai added inline comments.Oct 1 2021, 1:47 PM
llvm/test/CodeGen/PowerPC/ppc64-byval-multi-store.ll
1

Did you miss this comment or is the pre-commit and rebase coming?

stefanp updated this revision to Diff 376643.Oct 1 2021, 3:18 PM

Sorry, I completely missed the comment about pre-comitting the test case.
I have done that now and rebased.

nemanjai accepted this revision.Oct 5 2021, 3:36 PM

Significantly simpler. LGTM, thank you.

This revision is now accepted and ready to land.Oct 5 2021, 3:36 PM
This revision was automatically updated to reflect the committed changes.