This is an archive of the discontinued LLVM Phabricator instance.

GlobalISel: Partially fix handling of byval arguments
ClosedPublic

Authored by arsenm on Mar 6 2021, 9:02 AM.

Details

Summary

This was essentially ignoring byval and treating them as a pointer
argument which needed to be loaded from. This should copy the frame
index value to the virtual register, not insert a load from the frame
index into the pointer value.

For AMDGPU, this was producing a load from the byval pointer argument,
to a pointer used for the byval arguments. I do not understand how
AArch64 managed to work before since it appears to be similarly
broken.

We could also change the ValueHandler API to avoid the extra copy from
the frame index, since currently it returns a new register.

I believe there is still an issue with outgoing byval arguments. These
should have a copy inserted in case the callee decided to overwrite
the memory.

Diff Detail

Event Timeline

arsenm created this revision.Mar 6 2021, 9:02 AM
arsenm requested review of this revision.Mar 6 2021, 9:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 6 2021, 9:02 AM
Herald added a subscriber: wdng. · View Herald Transcript
paquette added inline comments.Mar 8 2021, 1:17 PM
llvm/lib/CodeGen/GlobalISel/CallLowering.cpp
632

Only check VA.isMemLoc once?

if (VA.isMemLoc()) {
  if (!Flags.isByVal()) {
    ...
    continue;
  }
  ...
  continue;
}
659

I don't think this check is necessary yet. Maybe better as an assert for now?

It turns out that we rarely use byval at all on AArch64, so that's why this wasn't found out earlier.

llvm/lib/CodeGen/GlobalISel/CallLowering.cpp
651

I assume a fix for this case is coming soon? Otherwise I think we should fall back here.

arsenm updated this revision to Diff 329950.Mar 11 2021, 6:14 AM

Remove assert

llvm/lib/CodeGen/GlobalISel/CallLowering.cpp
632

Maybe, but it increases the indentation for 2 cases that don't really have much in common

This revision is now accepted and ready to land.Mar 11 2021, 9:26 AM