This is an archive of the discontinued LLVM Phabricator instance.

[flang] Lowering LOC intrinsic
ClosedPublic

Authored by kkwli0 on Nov 23 2022, 6:41 AM.

Details

Diff Detail

Event Timeline

kkwli0 created this revision.Nov 23 2022, 6:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 23 2022, 6:41 AM
kkwli0 requested review of this revision.Nov 23 2022, 6:41 AM

Thanks for working on this, looks great for data "x", see my suggestion to support procedures.

flang/lib/Lower/IntrinsicCall.cpp
883
3758

This will probably no work when "x" is a procedure. I think "loc" extension has to work with procedures too (at least flang semantics accepts them).
Could you find a way to share the relevant parts of the the "genCLocOrCFunLoc" here https://github.com/llvm/llvm-project/blob/6ed85a62bdbc40a7d98205b2b3da1cb46120faf4/flang/lib/Lower/IntrinsicCall.cpp#L2684-L2695 with the loc implementation ?
The parts that gets the address should be similar.

kkwli0 marked 2 inline comments as done.Nov 23 2022, 1:12 PM
kkwli0 added inline comments.
flang/lib/Lower/IntrinsicCall.cpp
883

Will do.

3758

Good point. I missed the procedure case.

The advantage of the genCLocOrCFunLoc routine is that whether the argument is a procedure or not is passed in. loc intrinsic accepts both and getLoc needs some logic to determine if the incoming is fir.embox or fir.emboxproc assuming that the arg is already lowered to either. In order to share the portion of the code in genCLocOrCFunLoc, I did some re-arrangement of the assert statements.

kkwli0 updated this revision to Diff 477587.EditedNov 23 2022, 1:14 PM
kkwli0 marked 2 inline comments as done.

Address review comments:

  • sync up the argument keyword
  • share code with genCLocOrCFunLoc
kkwli0 added a comment.EditedNov 23 2022, 2:23 PM

@jeanPerier Thanks for the review. Should I include the update of Intrinsics.md (i.e. remove LOC from the unsupported list) in this patch?

jeanPerier accepted this revision.Nov 24 2022, 1:10 AM

@jeanPerier Thanks for the review. Should I include the update of Intrinsics.md (i.e. remove LOC from the unsupported list) in this patch?

That is a good idea, yes. Thanks.
Looks good otherwise, thanks for addressing my comments.

This revision is now accepted and ready to land.Nov 24 2022, 1:10 AM

@jeanPerier Thanks for the review. Should I include the update of Intrinsics.md (i.e. remove LOC from the unsupported list) in this patch?

That is a good idea, yes. Thanks.
Looks good otherwise, thanks for addressing my comments.

@jeanPerier Thanks a lot. I will include the change in Intrinsics.md in the commit.

This revision was automatically updated to reflect the committed changes.