This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel] Fix DIVREM combine from inserting a divrem before its operands' defs.
ClosedPublic

Authored by aemerson on Feb 18 2023, 2:26 PM.

Details

Summary

In some rare corner cases where in between the div/rem pair there's a def of
the second instruction's source (but a different vreg due to the combine's
eqivalence checks), it will place the DIVREM at the first instruction's point,
causing a use-before-def. There wasn't an obvious fix that stood out to me
without doing more involved analysis than a combine should really be doing.

Fixes issue #60516

I'm open to new suggestions on how to approach this, as I'm not too happy
at bailing out here. It's not the first time we run into issues with value liveness
that the DAG world isn't affected by.

Diff Detail

Event Timeline

aemerson created this revision.Feb 18 2023, 2:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 18 2023, 2:26 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
aemerson requested review of this revision.Feb 18 2023, 2:26 PM

This feels way more complicated than necessary but I don't really have a better suggestion

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1202

This will miss implicit defs if a non-G_* instruction ends up in the middle somehow

aemerson updated this revision to Diff 499062.Feb 21 2023, 12:52 AM

Check all defs.

cdevadas accepted this revision.Jun 3 2023, 6:04 PM
This revision is now accepted and ready to land.Jun 3 2023, 6:04 PM
This revision was landed with ongoing or failed builds.Jun 4 2023, 12:44 AM
This revision was automatically updated to reflect the committed changes.
efriedma added inline comments.
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1238

The reason this breaks is that you're using MI.getOperand(1).getReg(), and that doesn't dominate the insertion position, right? But OtherMI->getOperand(1).getReg() is equivalent, and if we're inserting at the position of OtherMI, it should dominate the insert position, I think?

aemerson added inline comments.Jun 5 2023, 9:19 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1238

Might have misunderstood your comment, but we're not necessarily inserting at OtherMI. We have to insert at whichever of the pair is first in block order to not break use-defs. If any def of second's sources are dominated by the first, then there's no safe place to choose (without more careful analysis).

efriedma added inline comments.Jun 5 2023, 10:19 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1238

We're inserting at either MI or OtherMI, right? If we're inserting at OtherMI, the operands of OtherMI dominate the insert point, so we can use the operands of OtherMI. If we're inserting at MI, the operands of MI dominate the insert point, so we can use the operands of MI. (The operands of MI are equivalent to the operands of OtherMI, so we can choose whichever operands make the dominance work.)

aemerson added inline comments.Jun 5 2023, 10:30 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1238

Oh right, I see what you mean now. Yes I think that works, I'll check it does and revert/change this.