Page MenuHomePhabricator

[ConstantHoisting] Do not attempt to hoist ImmArgs
AbandonedPublic

Authored by lenary on Sep 10 2019, 6:36 AM.

Details

Summary

llvm::canReplaceOperandWithVariable used to have logic so that no arguments to
intrinsic functions could be replaced with variables. This lead to
ConstantHoisting adding an exception for intrinsic functions when checking if it
could hoist a particular argument.

Now that we have ImmArg annotations on intrinsic parameters, we query
them directly, and remove the exception in ConstantHoisting. This means
that ConstantHoisting will no longer ask for the cost of an intrinsic
immediate argument, and nor will it attempt to hoist it.

The RISC-V target hit this issue in development (alluded to in
rL364046), where we wanted it to default to medium-to-high immediate
costs for most immediates (on all calls and intrinsics), but this caused
IR verifier errors because ConstantHoisting was hoisting an ImmArg.

For context, in RISC-V, unless the instruction takes a 12-bit integer
immediate, materialising an integer constant takes 1 instruction for a
12-bit immediate, 2 instructions for a 32-bit immediate, and up to 5
instructions for a 64-bit immediate.

This change does not cause differences in the tests, because all targets
have engineered their immediate cost models to give a low cost to most
intrinsic arguments by default, as we have had to do in the RISC-V
backend. This change will allow backends to have a more accurate default
cost for materialisng integer constants, without triggering errors in
the IR verifier.

Event Timeline

lenary created this revision.Sep 10 2019, 6:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 10 2019, 6:36 AM
asb added a subscriber: asb.Sep 11 2019, 2:22 AM
lenary abandoned this revision.Sep 11 2019, 3:45 AM

See D58233

Oh neat, that patch looks much more comprehensive.