For a given list of candidate instructions to sink, the sinking algorithm currently checks that each instruction has the same opcode and the same operand types (i.e., Instruction::isSameOperationAs). However, loaded values that are only every stored to memory are canonicalized by InstCombine to have integer types. Because this changes the operand types of the store instructions, this canonicalization can prevent store sinking in SimplifyCFG. So if the candidate instructions are stores, some of which have been canonicalized to operate on integer types while others have not, sinking will not occur. In such a situation, this patch tries to change the stores to operate on the same types so that they may be sunk.
Thanks for the quick feedback Danny/Davide. I definitely appreciate the point that SimplifyCFG may not be the best place for this kind of transformation. Davide, I assume the dedicated pass you're referring to is GVNSink? I don't think that pass is enabled yet (I haven't been closely following the progress, so I'm not sure what's holding it up at this point), but it's possible GVNSink would indeed catch the cases this patch does. I haven't tested that yet.
For the record, this patch resulted from rL320749. After sinking was moved later in the pipeline, I noticed that SimplifyCFG began missing sinking opportunities that it once caught due to the load/store canonicalization in InstCombine that I mentioned. So the intent wasn't really to extend SimplifyCFG, but to restore it's functionality.
I might consider holding my nose, if this restores something that was there.
I think the other two passes we have for doing sinking aren't currently enabled (GVNSink and Sink.cpp), although unless somebody puts effort in them this will always be a chicken-egg problem.
I'm not sure I'll have the time to review this patch carefully, I don't want to put you on the hook, but if you can consider an alternative, that would be great.
If you look at the original review you'll notice that I was fundamentally opposed to the change, see e.g. https://reviews.llvm.org/D38566#926530
(FWIW, it doesn't matter where we move sinking/hoisting there will be always some case that we can't get properly. We, of course should prioritize for the common case, at least IMHO).
I agree with that. I'll play around with GVNSink when I have a chance (this particular patch isn't exceptionally important to me right now). If we can enable GVNSink, perhaps we'll be able to completely remove sinking from SimplifyCFG.