This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel] Translate calls to memcpy et al to G_INTRINSIC_W_SIDE_EFFECTs and legalize later.
ClosedPublic

Authored by aemerson on Jul 17 2019, 3:42 PM.

Details

Summary

I plan on adding memcpy optimizations in the GlobalISel pipeline, but we can't do that unless we delay lowering to actual function calls. This patch changes the translator to generate G_INTRINSIC_W_SIDE_EFFECTS for these functions, and then have each target specify that using the new custom legalizer for intrinsics hook that they want it expanded it a libcall.

Diff Detail

Event Timeline

aemerson created this revision.Jul 17 2019, 3:42 PM
arsenm added inline comments.Jul 17 2019, 3:46 PM
llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
1141–1142

I would expect these to be passed through

aemerson updated this revision to Diff 210455.Jul 17 2019, 4:36 PM

Allow non-zero address spaces.

aemerson updated this revision to Diff 210462.Jul 17 2019, 5:01 PM

Factor out the code to get the intrinsic ID from an MI into a helper in Utils.

arsenm added inline comments.Jul 17 2019, 5:04 PM
llvm/lib/CodeGen/GlobalISel/Utils.cpp
408 ↗(On Diff #210462)

I've been using MI.getOperand(MI.getNumExplicitDefs()).getIntrinsicID() for this. I've been considering just adding getIntrinsicID() to MachineInstr directly

aemerson marked an inline comment as done.Jul 17 2019, 5:15 PM
aemerson added inline comments.
llvm/lib/CodeGen/GlobalISel/Utils.cpp
408 ↗(On Diff #210462)

I'll try that now.

aemerson updated this revision to Diff 210500.Jul 18 2019, 1:14 AM

Add a getIntrinsicID() method to MachineInstr.

aemerson updated this revision to Diff 210679.Jul 18 2019, 2:28 PM

Translate the volatile flag to an immediate instead of a G_CONSTANT.

Sigh, I think we also need to pass through the alignments of the src and dest parameters. SelectionDAG does this during building using MemCpyInst::{getDestAlignment(),getSrcAlignment()} and I think we currently lose that information during translation.

arsenm added inline comments.Jul 18 2019, 2:41 PM
llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
1148

Should these get 1/2 mem operands?

aemerson marked an inline comment as done.Jul 18 2019, 2:49 PM
aemerson added inline comments.
llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
1148

Maybe for the alignments? I'm not sure about the volatile flag though, it seems to apply to the whole intrinsic not to specific pointer.

arsenm added inline comments.Jul 18 2019, 2:53 PM
llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
1148

Size, alignment, and IR object. I. guess you would have to propagate the volatile to both

aemerson marked an inline comment as done.Jul 18 2019, 2:58 PM
aemerson added inline comments.
llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
1148

Worth a try. However the size would have to be a lie, since the memory accessed would be potentially unknown. Perhaps that doesn't matter as it's an intrinsic and any code dealing with it would need to know not to never try to use the size info.

arsenm added inline comments.Jul 18 2019, 3:01 PM
llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
1148

I guess you could preserve the size in the static case? I'm not sure if MMOs allow unknown size. Preserving the alignment and address space is important at least

aemerson updated this revision to Diff 210694.Jul 18 2019, 3:53 PM

Store alignment, volatile and addrspace info in MMOs.

aemerson marked an inline comment as done.Jul 18 2019, 3:54 PM
aemerson added inline comments.
llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
1148

I decided to leave the size as just 1 in every case, I think it's better than sometimes having correct information.

arsenm accepted this revision.Jul 18 2019, 4:49 PM
This revision is now accepted and ready to land.Jul 18 2019, 4:49 PM
This revision was automatically updated to reflect the committed changes.