This is an archive of the discontinued LLVM Phabricator instance.

Add -instcombine-code-sinking option
ClosedPublic

Authored by GBuella on Oct 1 2018, 1:19 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

GBuella created this revision.Oct 1 2018, 1:19 AM

Can you update this review with a summary that describes the problem your are trying to fix by disabling instruction sinking here?

lib/Transforms/InstCombine/InstructionCombining.cpp
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".

GBuella updated this revision to Diff 169375.Oct 12 2018, 5:18 AM

Adding constructor argument as well -- making it more similar to the ExpensiveCombines option.

GBuella updated this revision to Diff 169380.Oct 12 2018, 5:41 AM

I don't know if we want this passed through the constructor. ExpensiveCombines is there because its attached -O3. I think we should only do this when we have a use case for it.

Wouldn't it better to pull it out of instcombine into.. let say.. CodeSinkingPass?

Wouldn't it better to pull it out of instcombine into.. let say.. CodeSinkingPass?

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.

GBuella updated this revision to Diff 170164.Oct 19 2018, 1:57 AM

Back to having only a CLI option, no constructor argument.

This revision is now accepted and ready to land.Oct 19 2018, 11:36 AM

Can you update this review with a summary that describes the problem your are trying to fix by disabling instruction sinking here?

Hi, Intel Graphics Compiler compiles uses LLVM to optimize scalar IR code, which is vectorized later on. It is pretty difficult to explain the exact problems code sinking can create.

This revision was automatically updated to reflect the committed changes.
wuzish added a subscriber: wuzish.EditedAug 12 2019, 2:20 AM

Wouldn't it better to pull it out of instcombine into.. let say.. CodeSinkingPass?

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.

I also have a question that why lib/Transforms/Scalar/Sink.cpp is not enabled as default for all targets to do sinking instead of other separate places. @craig.topper

Herald added a project: Restricted Project. · View Herald TranscriptAug 12 2019, 2:20 AM