This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel] Add an operand to memory intrinsics to denote the "tail" marker.
ClosedPublic

Authored by aemerson on Sep 27 2019, 11:22 AM.

Details

Summary

We need to propagate this information from the IR in order to be able to safely do tail call optimizations on the intrinsics during legalization. Assuming it's safe to do tail call opt without checking for the marker isn't safe because the mem libcall may use allocas from the caller.

This adds an extra immediate operand to the end of the intrinsics and fixes the legalizer to handle it.

Diff Detail

Repository
rL LLVM

Event Timeline

aemerson created this revision.Sep 27 2019, 11:22 AM
paquette added a comment.EditedSep 27 2019, 11:34 AM

Can we also have a check for say, __memcpy_chk?

e.g.

define void @test_memcpy_chk() {
  %dst = bitcast %struct.T1* @t1 to i8*
  %src = bitcast %struct.T2* @t2 to i8*
  %ret = tail call i8* @__memcpy_chk(i8* %dst, i8* %src, i64 1824, i64 1824)
  ret void
}

This is correctly emitted as a normal call to memcpy by SelectionDAG, but incorrectly emitted as a tail call by GlobalISel.

paquette accepted this revision.Sep 27 2019, 3:30 PM

LGTM with test change

This revision is now accepted and ready to land.Sep 27 2019, 3:30 PM

I'm somewhat worried about this wrt. eventually verifying intrinsics have the correct number of operands

I'm somewhat worried about this wrt. eventually verifying intrinsics have the correct number of operands

At minimum the verifier should manually check this operand and manually verify these intrinsics

I'm somewhat worried about this wrt. eventually verifying intrinsics have the correct number of operands

At minimum the verifier should manually check this operand and manually verify these intrinsics

I'll add the verifier checks for this as well. If we want a different solution in general then let's start a separate discussion. Maybe we can explore adding more flags to MachineInstr, or dedicated G_CALL opcode that has a bitfield operand that propagates the flags from the CallInst.

I'm somewhat worried about this wrt. eventually verifying intrinsics have the correct number of operands

At minimum the verifier should manually check this operand and manually verify these intrinsics

I'll add the verifier checks for this as well. If we want a different solution in general then let's start a separate discussion. Maybe we can explore adding more flags to MachineInstr, or dedicated G_CALL opcode that has a bitfield operand that propagates the flags from the CallInst.

For AMDGPU I'm going to need a regbankselectable G_CALL at some point anyway

This revision was automatically updated to reflect the committed changes.