Page MenuHomePhabricator

Allow replacing intrinsic operands with variables
AcceptedPublic

Authored by arsenm on Feb 14 2019, 5:27 AM.

Details

Summary

Since intrinsics can now specify when an argument is required to be
constant, it is now OK to replace arguments with variables if they
aren't. This means intrinsics must now be accurately marked with
immarg.

Diff Detail

Event Timeline

arsenm created this revision.Feb 14 2019, 5:27 AM
arsenm updated this revision to Diff 186877.Feb 14 2019, 10:58 AM
arsenm added a reviewer: reames.

Handle some special cases I noticed in the gc intrinsics (particularly operands should still not be replaced if they are in the varargs section)

What protects target intrinsics that haven't been annotated with immarg yet?

What protects target intrinsics that haven't been annotated with immarg yet?

Nothing, these need to be accurate. I looked through the current set of in-tree target intrinsics, but don't see any comments or documentation stating certain arguments need to be constant so I hope we are OK there.

What protects target intrinsics that haven't been annotated with immarg yet?

Nothing, these need to be accurate. I looked through the current set of in-tree target intrinsics, but don't see any comments or documentation stating certain arguments need to be constant so I hope we are OK there.

It looks like this is sort of indirectly documented for the cases that have GCCBuiltins handled in clang. It's probably possible to put an assert somewhere in clang to find any of the intrinsics using the "I" modifier that don't have the matching immarg attribute. This of course won't really give 100% coverage though

No problems w/this from the GC side of things.

I don't really have any concerns about this from the perspective of correctness; any breakage will be obvious and easy to fix.

In terms of performance, I'm a little concerned we might not have an appropriate heuristic for profitability for all the callers of canReplaceOperandWithVariable. Even if it's technically legal, for example, pulling the length out of a memcpy forces it to be lowered to a library call, which could be a lot slower. (Even for non-intrinsic functions, we've had issues with this in the past; there was a bug reported at one point that we were pulling the shift amount out of an i256 shift, or something like that.)

I don't really have any concerns about this from the perspective of correctness; any breakage will be obvious and easy to fix.

In terms of performance, I'm a little concerned we might not have an appropriate heuristic for profitability for all the callers of canReplaceOperandWithVariable. Even if it's technically legal, for example, pulling the length out of a memcpy forces it to be lowered to a library call, which could be a lot slower. (Even for non-intrinsic functions, we've had issues with this in the past; there was a bug reported at one point that we were pulling the shift amount out of an i256 shift, or something like that.)

I could move the intrinsic check into ConstantHoisting for now; I would like canReplaceOperandWithVariable to report what's legal

It looks like the change is essentially a no-op for constant hoisting itself? Or maybe I'm misreading the code somehow? But sure, I'm fine with generally hoisting the check out of canReplaceOperandWithVariable, for all three of the current callers.

asb added a subscriber: asb.Sep 11 2019, 2:22 AM
reames accepted this revision.Oct 28 2019, 11:03 AM

LGTM

This revision is now accepted and ready to land.Oct 28 2019, 11:03 AM