This is an archive of the discontinued LLVM Phabricator instance.

[NFC][PowerPC] Add a function tryAndWithMask
ClosedPublic

Authored by steven.zhang on Dec 19 2019, 1:26 AM.

Details

Summary

I plan to add some more logic to optimize the code gen for "and" with mask. So, need to move all these logic into a function first and extend it later.

Diff Detail

Event Timeline

steven.zhang created this revision.Dec 19 2019, 1:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 19 2019, 1:26 AM
shchenz added inline comments.Dec 23 2019, 5:49 PM
llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
4677

Is there any reason lines 4678 ~ 4686 are left out from tryAndWithMask()?

steven.zhang marked an inline comment as done.Dec 23 2019, 6:32 PM
steven.zhang added inline comments.
llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
4677

It is rotate and mask, not and mask.

shchenz accepted this revision.Dec 23 2019, 7:09 PM

LGTM. Some minor comment, you can handle it as you like.

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
207

Maybe tryAndWithImmMask?

4677

I guess here we want to handle special case for And with a Imm mask, so if we change name tryAndWithMask() to tryAndWithImmMask, maybe we can also put these lines to that function also. What do you think? I am also ok to leave it as now.

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

I realize that most reviewers were on vacation during this patch was reviewed and committed. It is a bit too haste to commit this patch. Please add the comments here if have any concern. Sorry about this.

I think this refactoring doesn't really do anything to simplify the code or make it more readable. It just takes fairly complex logic and moves it into a separate function. Then subsequent patches will add even further complexity to this function. Overall, there isn't much of a gain from the refactoring.
What I think would make sense is to simplify the tryAndWithMask() to something conceptually similar to the following:

tryAndWithMask(...) {
  if (canBeCodeGenedAsSingleRLWINM())
    return produceRLWINMSDNode();
  if (canBeCodeGenedAsSingleRLWIMI())
    return produceRLWIMISDNode();
  if (canBeCodeGenedAsSingleRLDICL())
    return produceRLDICLSDNode();
  if (canBeCodeGenedAsPairOfRLDICL())
    return produceRLDICLPairSDNode();
  ...
  return SDValue();

Then the top level function would call this function to create an SDValue and return it if it is non-empty or break out of the switch if it is empty.

NeHuang added a subscriber: NeHuang.EditedJan 2 2020, 12:48 PM

Though the logic in this new function looks consistent with original code with optimized condition check, IMHO it is still not that readable to me. Agree with @nemanjai 's suggestion, adding conceptual functions for"SingleRLWINM", "SingleRLWIMI", "SingleRLDICL" and "PairOfRLDICL" will make it more readable. Also added comments for variable re-def issues.

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
4360

N->getOperand(0) is used at line 4361, 4362, 4373, 4388, 4389, 4404, 4405), and it is defined in line 4354. Can we use the defined variable (better name if necessary) to replace "N->getOperand(0)" ?

4361

Re-definition of Val. (Originally defined in Line 4354)

4401

Re-definition of MB and ME. (Originally defined in line 4355)

steven.zhang added a comment.EditedJan 2 2020, 6:39 PM

Yes, it is just moving the code into a new function to avoid too much code in the switch case. And it is definitely more clear if moving the condition into small functions. As there are several different conditions that could produce the same instruction with different arguments. I will do it as follows.

bool tryAndWithMask(...) {
   
   if (tryCodeGenAsSingleRLWINM())
       return true;
   if (tryCodeGenAsPairRLWINM())
      return true;
   ...
}

Maybe, the tryAndWithMask is NOT needed if we move the small pieces of code into functions.

amyk added a subscriber: amyk.Jan 2 2020, 7:55 PM

One minor comment from me. I also agree with what has been stated previously (to try to simplify tryAndWithMask()).

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
4348

If we are to add a new function, I think it is beneficial to add some doxygen comments for the function.