This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Reorder (shl (add/sub (shl x, C0), y), C1) -> (add/sub (shl x, C0 + C1), (shl y, C1))
ClosedPublic

Authored by goldstein.w.n on Jan 16 2023, 3:21 PM.

Diff Detail

Event Timeline

goldstein.w.n created this revision.Jan 16 2023, 3:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 16 2023, 3:21 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
goldstein.w.n requested review of this revision.Jan 16 2023, 3:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 16 2023, 3:21 PM

Subtract should be handled as well as add?

Subtract should be handled as well as add?

Yes, I'll update:
https://alive2.llvm.org/ce/z/4TVEIg

Add support for sub + update commit message

goldstein.w.n retitled this revision from [InstCombine] Reorder (shl (add (shl x, C0), y), C1) -> (add (shl x, C0 + C1), (shl y, C1)) to [InstCombine] Reorder (shl (add/sub (shl x, C0), y), C1) -> (add/sub (shl x, C0 + C1), (shl y, C1)).Jan 17 2023, 10:23 AM
goldstein.w.n edited the summary of this revision. (Show Details)

Subtract should be handled as well as add?

Done.

goldstein.w.n edited the summary of this revision. (Show Details)Jan 17 2023, 10:25 AM
spatel added inline comments.Jan 27 2023, 11:12 AM
llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
325–328

or add -> or add/sub

328

typo (it was already there but disguised) - "a a"

329

This function name is too specific now since we are not limited to logic ops. Not sure if there's a compact description for the set of possibilities. If not, just go with foldShiftOfShiftedBinOp().

352–355

This was strangely matched, so I reduced it:
c2ab7e2abd834d4ef7b0f277

It shouldn't change anything functionally here, but you may need to update/rebase to patch cleanly.

365

"Reorder" doesn't seem like the right description. I'd use the more literal "FirstShiftIsOp1" or something like that.

goldstein.w.n marked 5 inline comments as done.

Rebase, change func name, change reorder name, fix comments

spatel accepted this revision.Jan 27 2023, 11:47 AM

LGTM

This revision is now accepted and ready to land.Jan 27 2023, 11:47 AM

Either this or D140850 cause the failure: https://lab.llvm.org/buildbot/#/builders/183/builds/10447

Going to revert while I look into it.

Either this or D140850 cause the failure: https://lab.llvm.org/buildbot/#/builders/183/builds/10447

Going to revert while I look into it.

If that was the only builder that failed, I'd try again. It passed on the next try (before you reverted!):
https://lab.llvm.org/buildbot/#/builders/183
Sometimes buildbots just fail inexplicably. :)

goldstein.w.n added a comment.EditedJan 27 2023, 5:30 PM

Either this or D140850 cause the failure: https://lab.llvm.org/buildbot/#/builders/183/builds/10447

Going to revert while I look into it.

If that was the only builder that failed, I'd try again. It passed on the next try (before you reverted!):
https://lab.llvm.org/buildbot/#/builders/183
Sometimes buildbots just fail inexplicably. :)

Yeah, also unable to reproduce locally. Coming to conclusion its unrelated, cest la vie.
For recommit should I just apply re-apply the patches and prefix with "Recommit"/explain it was a false positive?

Also thanks for taking a look :)

Yeah, also unable to reproduce locally. Coming to conclusion its unrelated, cest la vie.
For recommit should I just apply re-apply the patches and prefix with "Recommit"/explain it was a false positive?

That should be fine - I usually just append a "(2nd try)" on the patch title, add a short explanation statement, then the original commit message.