Can you update this review with a summary that describes the problem your are trying to fix by disabling instruction sinking here?
|123 ↗||(On Diff #167678)|
This could use a longer description and a comment block explaining why it's an option. I also don't think it needs to be hidden. And for consistency with similar options, I would name this variable "EnableCodeSinking".
Absolutely. This code doesn't belong in InstCombine. This code isn't even the code we would use if we had a real sinking pass. This is a very simple sinking algorithm. It only moves code to an immediate successor and it has a bogus one use check in it. I think we should have a real sinking pass. lib/Transforms/Scalar/Sink.cpp is probably a good starting point, I believe its only used by one target today. I think adding a disable here is a good first step towards being able to experiment with a real sinking pass.
lib/Transforms/Scalar/Sink.cpp is probably a good starting point
I did an experiment with this recently. It seems to be in a pretty good state already; it reduced codesize for an ARM codebase I was looking at by about by about 0.1%. Haven't done any careful benchmarking, though.
Adding the command-line flag seems like enough to experiment. Not sure adding a flag to createInstructionCombiningPass() is really productive; it won't be useful once we turn off sinking by default.