[PATCH] D76294: [PowerPC][Future] Add initial support for PC Relative addressing to get block address

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai accepted this revision.
nemanjai added a comment.
This revision is now accepted and ready to land.

LGTM aside from some nits that should be easily addressable on the commit.



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:2557
+  BlockAddressSDNode *BASDN = dyn_cast<BlockAddressSDNode>(N.getNode());
+  bool BlockAddress = BASDN && BASDN->getTargetFlags() == PPCII::MO_PCREL_FLAG;
+  if (ConstPool || Global || JumpTable || BlockAddress) {
----------------
This is starting to get unnecessary. There is no reason to have all the different `bool` variables.
Something like this might be more readable:
```
  bool ValidPCRelNode = false;
#define VALIDATE_PCREL_NODE(TYPE)                                              \
  {                                                                            \
    TYPE *PCRelCand = dyn_cast<TYPE>(N);                                       \
    if (PCRelCand && PCRelCand->getTargetFlags() == PPCII::MO_PCREL_FLAG)      \
      ValidPCRelNode = true;                                                   \
  }
  VALIDATE_PCREL_NODE(ConstantPoolSDNode);
  VALIDATE_PCREL_NODE(GlobalAddressSDNode);
  VALIDATE_PCREL_NODE(JumpTableSDNode);
  VALIDATE_PCREL_NODE(BlockAddressSDNode);
  if (ValidPCRelNode) {
    Base = N;
    return true;
  }
  return false;
#undef VALIDATE_PCREL_NODE
```

Or define a template such as:
```
template <typename Ty> static bool isValidPCRelNode(SDValue N) {
  Ty *PCRelCand = dyn_cast<Ty>(N);
  return PCRelCand && PCRelCand->getTargetFlags() == PPCII::MO_PCREL_FLAG;
}
```
And then this function is just:
```
  if (isValidPCRelNode<ConstantPoolSDNode>(N) ||
      isValidPCRelNode<GlobalAddressSDNode>(N) ||
      isValidPCRelNode<JumpTableSDNode>(N) ||
      isValidPCRelNode<BlockAddressSDNode>(N)) {
    Base = N;
    return true;
  }
  return false;
```

My preference being for the latter.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:2895
   if (Subtarget.is64BitELFABI() || Subtarget.isAIXABI()) {
+    if (Subtarget.hasPCRelativeMemops()) {
+      SDLoc DL(BASDN);
----------------
amyk wrote:
> I think it would be good if we can put a comment above briefly saying what happens when we have PC relative mem ops.
Remove this from the AIX block as there is no PC-Rel on AIX.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76294/new/

https://reviews.llvm.org/D76294