This is an archive of the discontinued LLVM Phabricator instance.

[DAGCombiner] Make it so that `udiv` can be folded with `(select c, NonZero, 1)`
ClosedPublic

Authored by goldstein.w.n on Apr 25 2023, 2:27 PM.

Details

Summary

This is done by allowing speculation of udiv if we can prove the
denominator is non-zero.

https://alive2.llvm.org/ce/z/VNCt_q

Diff Detail

Event Timeline

goldstein.w.n created this revision.Apr 25 2023, 2:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 25 2023, 2:27 PM
goldstein.w.n requested review of this revision.Apr 25 2023, 2:27 PM
pengfei added inline comments.Apr 25 2023, 8:34 PM
llvm/include/llvm/CodeGen/SelectionDAG.h
2345

Why don't refactor it directly?

goldstein.w.n added inline comments.Apr 25 2023, 9:18 PM
llvm/include/llvm/CodeGen/SelectionDAG.h
2345

I figured they have seperate purposes. The Opcode version is checking whether an opcode is unconditionally safe to speculate, node is asking about this specific case. But I'll check the other usages and see if the refactored version makes sense.

goldstein.w.n marked an inline comment as done.

just refactor isSafeToSpeculativelyExecute

Did general refactror.

craig.topper added a comment.EditedApr 25 2023, 10:14 PM

Did general refactror.

Does that increase the review burden? Now we have to audit the other transforms to make sure the don't change operand 1 in a way that would be unsafe?

Plus we should have additional tests?

Did general refactror.

Does that increase the review burden? Now we have to audit the other transforms to make sure the don't change operand 1 in a way that would be unsafe?

Plus we should have additional tests?

Maybe we can use another NFC patch to refactor the interface only?

goldstein.w.n added a comment.EditedApr 26 2023, 8:38 AM

Did general refactror.

Does that increase the review burden? Now we have to audit the other transforms to make sure the don't change operand 1 in a way that would be unsafe?

Plus we should have additional tests?

Maybe we can use another NFC patch to refactor the interface only?

Sure. I'll leave as previous version in this patch and make the refactor a follow up.
Edit: Looking at some of the other locations more closely. I think it makes
sense to leave as a seperate API. isSafeToSpeculativelyExecute is used to query
about the opcode without knowing the operands so serves a different purpose.

Did general refactror.

Does that increase the review burden? Now we have to audit the other transforms to make sure the don't change operand 1 in a way that would be unsafe?

Plus we should have additional tests?

Maybe we can use another NFC patch to refactor the interface only?

It wouldn't be NFC. It would make some UDIV instructions supported by additional transforms.

Go back to original logic

RKSimon added inline comments.May 13 2023, 3:47 AM
llvm/include/llvm/CodeGen/SelectionDAG.h
2359

Add doxygen description

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
2436–2437

update comment

RKSimon retitled this revision from [DAGCombiner] Make is so that `udiv` can be folded with `(select c, NonZero, 1)` to [DAGCombiner] Make it so that `udiv` can be folded with `(select c, NonZero, 1)`.May 13 2023, 4:52 AM
goldstein.w.n marked 2 inline comments as done.

Fix comments

pengfei accepted this revision.May 18 2023, 4:46 AM

LGTM.

llvm/include/llvm/CodeGen/SelectionDAG.h
2359

safe

This revision is now accepted and ready to land.May 18 2023, 4:46 AM
xbolva00 added inline comments.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
2438

but UREM is not handled above..

goldstein.w.n added inline comments.May 18 2023, 2:53 PM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
2438

Well the comment still holds true, but indeed could include urem in this patch. Will update.

Sorry for long delay on this, going to push this shortly.