Page MenuHomePhabricator

[GlobalISel] Add setting of pointer flags to ArgInfo
AbandonedPublic

Authored by ehjogab on Aug 19 2020, 1:42 AM.

Details

Summary

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

Diff Detail

Event Timeline

ehjogab created this revision.Aug 19 2020, 1:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 19 2020, 1:42 AM
ehjogab requested review of this revision.Aug 19 2020, 1:42 AM

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?

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?

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 abandoned this revision.Aug 19 2020, 11:53 PM

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!