[GlobalISel] Add setting of pointer flags to ArgInfo

[ehjogab]

Summary

When invoking CallLowering::setArgFlags() with a pointer type, it did
not properly set the pointer flags.

[arsenm]

I'm curious why you need this. These fields are mostly a hack for SelectionDAG?

llvm/lib/CodeGen/GlobalISel/CallLowering.cpp

86–90

dyn_cast instead of isPointerTy and cast?

[ehjogab]

In D86200#2226077, @arsenm wrote:

I'm curious why you need this. These fields are mostly a hack for SelectionDAG?

In our target argument pointers are converted into MVT::iPTRs, but in light of your comment that might be a bug in our backend. Could you comment on how, and if, MVT::iPTR are suppose to be used in GlobalISel, or are they only intended for SelectionISel?

[arsenm]

In D86200#2226337, @ehjogab wrote:

In D86200#2226077, @arsenm wrote:

I'm curious why you need this. These fields are mostly a hack for SelectionDAG?

In our target argument pointers are converted into MVT::iPTRs, but in light of your comment that might be a bug in our backend. Could you comment on how, and if, MVT::iPTR are suppose to be used in GlobalISel, or are they only intended for SelectionISel?

Ideally MVT wouldn't be used at all, and we're just using the existing SelectionDAG calling convention lowering with its ties to MVT as a crutch. LLT naturally preserves that the value is a pointer with address space and size

[ehjogab]

In D86200#2226354, @arsenm wrote:

In D86200#2226337, @ehjogab wrote:

In D86200#2226077, @arsenm wrote:

I'm curious why you need this. These fields are mostly a hack for SelectionDAG?

In our target argument pointers are converted into MVT::iPTRs, but in light of your comment that might be a bug in our backend. Could you comment on how, and if, MVT::iPTR are suppose to be used in GlobalISel, or are they only intended for SelectionISel?

Ideally MVT wouldn't be used at all, and we're just using the existing SelectionDAG calling convention lowering with its ties to MVT as a crutch. LLT naturally preserves that the value is a pointer with address space and size

I see. I compared the implementation we have against that of AArch64, and I notice a small difference in when splitting the argument: Both invoke ComputeValueVTs(), but if the number of splits is 1 (meaning there is no splitting) then AArch64 takes the single value produced by ComputeValueVTs() whereas our backend simply uses what was input to ComputeValueVTs. This means that, if the value was a pointer, our backend retains the pointer value whereas AArch64 uses the converted value, which is an integer. I actually think that the correct way is to use the converted value instead of retaining the pointer value, which means that this patch is redundant and hence to be abandoned.

Thanks for the feedback!

llvm/lib/CodeGen/GlobalISel/CallLowering.cpp

 //===-- lib/CodeGen/GlobalISel/CallLowering.cpp - Call lowering -----------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 ///

 }
 
 template <typename FuncInfoTy>
 void CallLowering::setArgFlags(CallLowering::ArgInfo &Arg, unsigned OpIdx,
                                const DataLayout &DL,
                                const FuncInfoTy &FuncInfo) const {
   auto &Flags = Arg.Flags[0];
   const AttributeList &Attrs = FuncInfo.getAttributes();
+  if (Arg.Ty->isPointerTy()) {
+    Flags.setPointer();
+    Flags.setPointerAddrSpace(
+        cast<PointerType>(Arg.Ty)->getAddressSpace());
+  }
   if (Attrs.hasAttribute(OpIdx, Attribute::ZExt))
     Flags.setZExt();
   if (Attrs.hasAttribute(OpIdx, Attribute::SExt))
     Flags.setSExt();
   if (Attrs.hasAttribute(OpIdx, Attribute::InReg))
     Flags.setInReg();
   if (Attrs.hasAttribute(OpIdx, Attribute::StructRet))
     Flags.setSRet();