This is an archive of the discontinued LLVM Phabricator instance.

[InstSimplify] Use dominate condtion to simplify instructions
ClosedPublic

Authored by bcl5980 on Nov 22 2022, 9:16 PM.

Diff Detail

Event Timeline

bcl5980 created this revision.Nov 22 2022, 9:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 22 2022, 9:16 PM
bcl5980 requested review of this revision.Nov 22 2022, 9:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 22 2022, 9:16 PM

I will commit the test as baseline if reviewers look good for the test.

There's a question of where this kind of transform belongs - CVP, SCCP?

This seems fine to me, but it should probably be split off into a helper function. We could do the same for srem/urem? sdiv/udiv with a slight adjustment? any other opcodes?

llvm/test/Transforms/InstSimplify/domcondition.ll
36

Something went wrong - this should not simplify?

68

xor -> sub?

There's a question of where this kind of transform belongs - CVP, SCCP?

I guess if one of the compare value is constant, it should be IPSCCP. If both of them are not constant, it should be CVP.

This seems fine to me, but it should probably be split off into a helper function. We could do the same for srem/urem? sdiv/udiv with a slight adjustment? any other opcodes?

Yeah, you are right. div/rem should work also.

llvm/test/Transforms/InstSimplify/domcondition.ll
36

Yeah, this is a negative test. The condition is icmp ne.

bcl5980 updated this revision to Diff 477491.Nov 23 2022, 6:51 AM
bcl5980 retitled this revision from [InstSimplify] Use dominate condtion to simplify xor/sub to [InstSimplify] Use dominate condtion to simplify instructions.
bcl5980 added inline comments.Nov 23 2022, 6:59 AM
llvm/lib/Analysis/InstructionSimplify.cpp
762

I'm a little worry about the and/or part. Recursive call to isImpliedByDomCondition looks a little heavy.
I return Op1 here is because Op1 can be constant value but not sure it has any side effect or not.

spatel added inline comments.Nov 23 2022, 10:58 AM
llvm/lib/Analysis/InstructionSimplify.cpp
744–745

Put a descriptive comment on this function like:

/// Test if there is a dominating equivalence condition for the
/// two operands. If there is, try to reduce the binary operation
/// between the two operands. 
/// Example: Op0 - Op1 --> 0 when Op0 == Op1
762

Did you run any compile-time-tracker experiments?
We could leave those out of the initial patch, add a TODO, and then put them back in a follow-up patch to reduce risk.

911–912

If there's a good block comment for the new function, comments like this at the callers are probably not needed.

llvm/test/Transforms/InstSimplify/domcondition.ll
20

The mul in these tests could be removed?

It would be better to still have at least one negative test. We want to make sure the implication is not inverted somehow.

Go ahead and pre-commit the baseline tests, so we just show diffs in this patch.

bcl5980 updated this revision to Diff 477661.Nov 23 2022, 5:09 PM

Address comments.

bcl5980 added inline comments.Nov 23 2022, 6:14 PM
llvm/lib/Analysis/InstructionSimplify.cpp
762

Hi @spatel, can you help to run the and/or part to see the compile-time-tracker result? I have no privilege to test.

spatel added inline comments.Nov 24 2022, 8:08 AM
llvm/lib/Analysis/InstructionSimplify.cpp
762

Sure - I created a branch from current main and applied this patch to it:
https://llvm-compile-time-tracker.com/compare.php?from=bf7f87e62c3c8016886dc722eeae9062624a73ca&to=89cd9e520cfa3f67291c8ab7c9addad058a2a05c&stat=instructions:u
(note: you may be able to request permission to run experiments like that by sending a note to @nikic - it is a great resource; thanks!)

There seems to be a slight increase in time, but it could also be in the noise. If you want to compare vs. a CVP solution, I think it would be interesting to know the result.

spatel added inline comments.Nov 24 2022, 8:12 AM
llvm/lib/Analysis/InstructionSimplify.cpp
762

Sorry - after re-reading the comment, I realize I didn't run the experiment that was requested.

I'll add and/or in another commit.

spatel added inline comments.Nov 24 2022, 8:35 AM
llvm/lib/Analysis/InstructionSimplify.cpp
762

https://llvm-compile-time-tracker.com/?config=NewPM-O3&stat=instructions%3Au&remote=rotateright

So there does seem to be a noticeable cost to handling more instructions. This again suggests we might do better by creating a CVP patch.

bcl5980 added inline comments.Nov 24 2022, 2:04 PM
llvm/lib/Analysis/InstructionSimplify.cpp
762

It looks the if we don't propagate and/or the compile time is acceptable.
I'm sorry but I don't know how to add the similar code into CVP.
It looks for now CVP only detect constant/constant range based on ValueLatticeElement.

bcl5980 added inline comments.Nov 24 2022, 10:03 PM
llvm/lib/Analysis/InstructionSimplify.cpp
762

Another headache thing I find here is the pass sequence. SimplifyCFGPass will convert the test case to select at very early time. Even if we add the similar code into CVP it still can't detect the select.
Maybe we need to limit the dominate condition search to only when MaxRecurse == RecursionLimit .
Recursive search dominate condition can't get any improvement.

bcl5980 updated this revision to Diff 477857.Nov 24 2022, 10:08 PM
bcl5980 added inline comments.
llvm/lib/Analysis/InstructionSimplify.cpp
762

Hi @spatel, can you help to test the new patch with limited recursive call?

I made a small adjustment to pass the MaxRecurse through to div/rem as well.

There doesn't appear to be any real cost even if we include and/or in the opcodes now:
https://llvm-compile-time-tracker.com/compare.php?from=27b861a1fd271459e8109418f79293732770bc04&to=7b84f6922df17d6609cffa5c6d6d7ea2083341b8&stat=instructions:u

See the pair of commits ending with:
https://github.com/llvm/llvm-project/commit/7b84f6922df17d6609cffa5c6d6d7ea2083341b8

I made a small adjustment to pass the MaxRecurse through to div/rem as well.

There doesn't appear to be any real cost even if we include and/or in the opcodes now:
https://llvm-compile-time-tracker.com/compare.php?from=27b861a1fd271459e8109418f79293732770bc04&to=7b84f6922df17d6609cffa5c6d6d7ea2083341b8&stat=instructions:u

See the pair of commits ending with:
https://github.com/llvm/llvm-project/commit/7b84f6922df17d6609cffa5c6d6d7ea2083341b8

So how do you think the patch now? Do we need to add and/or in this patch? Or do we need to find a way to work on CVP/SCCP?

I made a small adjustment to pass the MaxRecurse through to div/rem as well.

There doesn't appear to be any real cost even if we include and/or in the opcodes now:
https://llvm-compile-time-tracker.com/compare.php?from=27b861a1fd271459e8109418f79293732770bc04&to=7b84f6922df17d6609cffa5c6d6d7ea2083341b8&stat=instructions:u

See the pair of commits ending with:
https://github.com/llvm/llvm-project/commit/7b84f6922df17d6609cffa5c6d6d7ea2083341b8

So how do you think the patch now? Do we need to add and/or in this patch? Or do we need to find a way to work on CVP/SCCP?

This looks almost good to me now - I would just make the change to MaxRecurse that I tried. That makes the behavior consistent for all opcodes. I think we should handle ‘and’ and ‘or’ for completeness since it doesn’t seem to have any extra cost. If you want to leave it as a TODO in this patch, that is ok.

bcl5980 updated this revision to Diff 478035.Nov 25 2022, 6:06 PM

Add recursive count for divrem.
And/Or are also added into simplifyByDomEq.

spatel accepted this revision.Nov 26 2022, 4:36 AM

LGTM

This revision is now accepted and ready to land.Nov 26 2022, 4:36 AM
This revision was landed with ongoing or failed builds.Nov 26 2022, 5:42 AM
This revision was automatically updated to reflect the committed changes.
bcl5980 added a comment.EditedNov 27 2022, 4:05 AM

Note:
This pattern will be triggered in
SpecCPU2017\benchspec\CPU\502.gcc_r\src\tree-ssa-alias.c
SpecCPU2017\benchspec\CPU\510.parest_r\src\source\lac\block_sparse_matrix.cc
SpecCPU2017\benchspec\CPU\538.imagick_r\src\magick\property.c