Page MenuHomePhabricator

[X86] Fix a bug that when lowering byval argument
ClosedPublic

Authored by LiuChen3 on Jul 5 2020, 6:20 AM.

Details

Summary

When an argument has 'byval' attribute and should be passed on the stack according calling convention,
a stack copy would be emitted twice. This will cause the real value will be put into stack where the pointer should be passed.

Diff Detail

Event Timeline

LiuChen3 created this revision.Jul 5 2020, 6:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2020, 6:20 AM
craig.topper added inline comments.Jul 5 2020, 8:54 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
4042

Why do we need a new variable? Can we just pass isByVal to the function?

LiuChen3 marked an inline comment as done.Jul 5 2020, 5:48 PM
LiuChen3 added inline comments.
llvm/lib/Target/X86/X86ISelLowering.cpp
4042

Yes, we can. I thought it will reduce readability, so I add a new variable.

LiuChen3 updated this revision to Diff 275619.Jul 6 2020, 2:36 AM

Remove redundant variables

LuoYuanke added inline comments.Jul 6 2020, 6:04 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
3774

Let me check below 2 rule is right or not.

  1. On linux when the byval attribute is set, it indicate copy the value that point by the pointer to the parameter stack.
  2. On window when the byval attribute is set, it indicate that allocate temporary object in caller, copy the value to the temporary, and store the temporary pointer (which point to the temporary object) to the parameter stack.

On linux, the VA.getLocInfo() is CCValAssign::Full, and on windows is the VA.getLocInfo() is CCValAssign::Indirect.

So I think we can just check the VA.getLocInfo(). If VA.getLocInfo() is CCValAssign::Indirect, we can NOT copy object. Instead we just restore the pointer.
(Flags.isByVal() && VA.getLocInfo() != CCValAssign::Indirect)

craig.topper added inline comments.Jul 6 2020, 1:04 PM
llvm/lib/Target/X86/X86ISelLowering.cpp
3774

The hasCopy should just be isByVal with no inversion and the Flags.isByVal() should be removed and replaced with isByVal. isByVal replaced the version in Flags.

Or what Yuanke said might work.

LiuChen3 updated this revision to Diff 275905.Jul 6 2020, 10:19 PM

Address the comments

This revision is now accepted and ready to land.Jul 6 2020, 11:40 PM
This revision was automatically updated to reflect the committed changes.