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
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
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.)
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.