This is an archive of the discontinued LLVM Phabricator instance.

[fir] Add fir character builder
ClosedPublic

Authored by clementval on Dec 1 2021, 12:23 PM.

Details

Summary

This patch adds the FIR builder to generate the numeric intrinsic
runtime call.

This patch is part of the upstreaming effort from fir-dev branch.

Co-authored-by: Jean Perier <jperier@nvidia.com>
Co-authored-by: mleair <leairmark@gmail.com>

Diff Detail

Event Timeline

clementval created this revision.Dec 1 2021, 12:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 1 2021, 12:23 PM
clementval requested review of this revision.Dec 1 2021, 12:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 1 2021, 12:23 PM
schweitz added inline comments.Dec 1 2021, 3:02 PM
flang/lib/Optimizer/Builder/Runtime/Character.cpp
207

It looks like some of these have doxygen comments in both the .h and .cpp file.

rovka added inline comments.Dec 1 2021, 8:20 PM
flang/include/flang/Optimizer/Builder/Runtime/Character.h
50

Nit: Is this actually used from outside this module, or can it be downgraded to a helper in the implementation file?

flang/lib/Optimizer/Builder/Runtime/Character.cpp
106

Please add a test for this (and check the Alloca and Store in it too).

112

I'm a bit confused, aren't descriptors the main thing we're supposed to handle here?
Also, why is this calling the other overload? I would expect it to generate calls to CharacterCompareScalar, not CharacterCompareScalarN.

201

This can also take source arguments.

244

This can also take source args.

285

Nit: I think you can move the definition to the top of the file (i.e. merge it with the declaration).

clementval marked 5 inline comments as done.Dec 2 2021, 9:47 AM
clementval added inline comments.
flang/lib/Optimizer/Builder/Runtime/Character.cpp
112

Lowering is actually done is a different way at the moment so we don't need this. If lowering stays as of today then this could be turn into an assert but we might update it in the future so a TODO seems the right thing right now.

201

Not sure In understand your comment. Source args are extracted from loc and added as arguments in genCharacterSearch

244

Yeah there are extracted from loc in genCharacterSearch.

clementval updated this revision to Diff 391368.Dec 2 2021, 9:48 AM
clementval marked 2 inline comments as done.

Address review comments

flang/lib/Optimizer/Builder/Runtime/Character.cpp
106

Seems pretty complicated to create the right data structure by hand to trigger the alloca creation. bbc would be of great help here.

rovka accepted this revision.Dec 3 2021, 5:05 AM

Right, LGTM.

This revision is now accepted and ready to land.Dec 3 2021, 5:05 AM
This revision was automatically updated to reflect the committed changes.