Page MenuHomePhabricator

[NFC][InstSimplify] Refactoring ThreadCmpOverSelect function
ClosedPublic

Authored by dendibakh on Dec 6 2019, 4:55 PM.

Details

Summary

Removed code duplication in ThreadCmpOverSelect and broke it
into several smaller functions for reusing them.

Diff Detail

Event Timeline

dendibakh created this revision.Dec 6 2019, 4:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 6 2019, 4:55 PM

@spatel, do you think I should add more reviewers?

@spatel, do you think I should add more reviewers?

Sure - adding others who might have feedback is always a good thing. My review queue is longer than I'd like, but I'm hoping to take a look within a day or two.

xbolva00 added inline comments.Dec 9 2019, 11:56 AM
llvm/lib/Analysis/InstructionSimplify.cpp
147

It would be great to improve these original comments a bit.

148

return TrueOrFalse;

154

else if (!SimplifiedCmp && isSameCompare(Cond, Pred, LHS, RHS))) {

return TrueOrFalse;

}

156

return nullptr;

dendibakh updated this revision to Diff 232914.Dec 9 2019, 12:53 PM

Fixed comments by David.

dendibakh marked 4 inline comments as done.Dec 9 2019, 12:56 PM
dendibakh added inline comments.
llvm/lib/Analysis/InstructionSimplify.cpp
156

Here SimplifiedCmp should stay, because composed comparison can be simplified to something other than true or false, so we should return it.

xbolva00 added inline comments.Dec 9 2019, 4:07 PM
llvm/lib/Analysis/InstructionSimplify.cpp
156

Oh, okay :)

xbolva00 added inline comments.Dec 9 2019, 4:09 PM
llvm/lib/Analysis/InstructionSimplify.cpp
156

(maybe you can add a code comment too)

xbolva00 added inline comments.Dec 9 2019, 4:10 PM
llvm/lib/Analysis/InstructionSimplify.cpp
158

btw, do you plan to use this helper functions in a future patch? For now, I dont think we really need them in this patch.

dendibakh marked an inline comment as done.Dec 9 2019, 4:36 PM
dendibakh added inline comments.
llvm/lib/Analysis/InstructionSimplify.cpp
158

btw, do you plan to use this helper functions in a future patch?

Yes

For now, I dont think we really need them in this patch.

Well, I believe it makes the code a little bit cleaner anyway. :)

This looks fine I think.

llvm/lib/Analysis/InstructionSimplify.cpp
158

Ok, it is fine.

spatel added inline comments.Dec 11 2019, 6:42 AM
llvm/lib/Analysis/InstructionSimplify.cpp
144

Should the type of the last arg be Constant?
Constant *TrueOrFalse

147

Agree - there's no context for what we're doing at this point. IIUC, the original pattern is:
cmp Pred (select Cond, TV, FV), RHS

And we're checking if this simplifies:
cmp Pred TV/FV, RHS

What happens after that is complicated...it might help to add an example?

dendibakh marked an inline comment as done.

Updated according to comments by Sanjay.

dendibakh marked an inline comment as done.Dec 12 2019, 11:17 AM
spatel accepted this revision.Dec 12 2019, 12:43 PM

LGTM

This revision is now accepted and ready to land.Dec 12 2019, 12:43 PM
This revision was automatically updated to reflect the committed changes.