Page MenuHomePhabricator

[SimplifyCFG] Try to change store operation type when sinking
AbandonedPublic

Authored by mssimpso on Jan 12 2018, 12:24 PM.

Details

Summary

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.

Diff Detail

Event Timeline

mssimpso created this revision.Jan 12 2018, 12:24 PM
davide requested changes to this revision.Jan 12 2018, 1:42 PM
davide added a subscriber: davide.

+1, we have a dedicated sinking pass. Not blaming on you, but unfortunately SimplifyCFG is becoming a kitchen sink, let's try to keep it at least half-sane :)

This revision now requires changes to proceed.Jan 12 2018, 1:42 PM

(As I already previously advocated), instcombine & simplifyCFG shouldn't do any hoisting/sinking whatsoever, but I only had limited energy to fight this battle.

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.

mssimpso abandoned this revision.Jan 12 2018, 2:51 PM

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