Page MenuHomePhabricator

[InstCombine] Push negation through multiply (PR44234)
ClosedPublic

Authored by nikic on Jan 25 2020, 1:55 AM.

Details

Summary

Fixes https://bugs.llvm.org/show_bug.cgi?id=44234 by adding multiply support to freelyNegateValue(). Only one of the operands needs to be negatible, so this still fits within the framework.

Diff Detail

Event Timeline

nikic created this revision.Jan 25 2020, 1:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 25 2020, 1:55 AM
nikic marked an inline comment as done.Jan 25 2020, 2:00 AM
nikic added inline comments.
llvm/test/Transforms/InstCombine/and-or-icmps.ll
207 ↗(On Diff #240367)

We have a regression here... the relevant difference is that the %B7 neg gets reassociated to act on %B23 instead, which is then pushed through to the %B11 udiv, which is a sext at that point and gets folded to zext. That somehow prevents other folds. As this is a fuzzer generated test case I'm not particularly worried about this, but can take a closer look if people think this is important.

Thanks on working on this

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
940

Not related to this change:

Should we worry about recursion depth here? Probably not a issue in practise..

llvm/test/Transforms/InstCombine/sub.ll
701–702

commuted

nikic updated this revision to Diff 240679.Jan 27 2020, 2:03 PM
nikic marked an inline comment as done and an inline comment as not done.

Fix typo

nikic marked an inline comment as done.Jan 27 2020, 2:08 PM
nikic added inline comments.
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
940

Due to the one-use restriction (which prevents degenerating to exponential walks) I'm not really worried about recursion here. It could in theory visit many instructions (if you have a thousand multiplies in a chain...), but will never result in super-linear runtime, which I think is the main concern.

But I can add a depth limit if desired.

nikic updated this revision to Diff 241550.Thu, Jan 30, 12:46 PM

Rebase over D73411.

Looks like the regression in and-or-icmps.ll wasn't actually caused by the IR difference itself, but the worklist order change caused by performing the negation. After rebasing over D73411 the regression disappeared :)

xbolva00 accepted this revision.Thu, Jan 30, 1:18 PM

Great! LG.

@spatel ?

This revision is now accepted and ready to land.Thu, Jan 30, 1:18 PM
spatel accepted this revision.Thu, Jan 30, 2:08 PM

LGTM

This revision was automatically updated to reflect the committed changes.