This is an archive of the discontinued LLVM Phabricator instance.

[fir] Add fir.select_case conversion
ClosedPublic

Authored by clementval on Nov 9 2021, 6:51 AM.

Details

Summary

The fir.select_case operation is converted to a if-then-else ladder.

Conversion of fir.select_case operation with character is not
implemented yet.

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

Co-authored-by: Eric Schweitz <eschweitz@nvidia.com>

Diff Detail

Event Timeline

clementval created this revision.Nov 9 2021, 6:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 9 2021, 6:51 AM
Herald added a subscriber: mehdi_amini. · View Herald Transcript
clementval requested review of this revision.Nov 9 2021, 6:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 9 2021, 6:51 AM
mehdi_amini added inline comments.Nov 9 2021, 10:24 AM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
784

Can you expand a bit on the doc here? This isn't a 1-1 conversion, so it seems worth spelling it out.

806

The & reference hear is misleading, I don't think it ever gets modified and Attribute is a value type

847

Many types to spell out instead of auto in there, can you update please?

clementval updated this revision to Diff 385960.Nov 9 2021, 1:40 PM

Address review comments

clementval marked 3 inline comments as done.Nov 9 2021, 1:41 PM
clementval added inline comments.
flang/lib/Optimizer/CodeGen/CodeGen.cpp
847

Sorry, I forgot to go through the auto in this pattern. Should be done.

LGTM, would be nice if someone else would also have a look at it.

LGTM, would be nice if someone else would also have a look at it.

Agree with @mehdi_amini -- looks good. One minor typo in the fir.select_casefirst comments: "A a point value..."

clementval marked an inline comment as done.Nov 10 2021, 12:16 AM

LGTM, would be nice if someone else would also have a look at it.

Agree with @mehdi_amini -- looks good. One minor typo in the fir.select_casefirst comments: "A a point value..."

Thanks I'll fix that. Can someone approve it then?

flang/lib/Optimizer/CodeGen/CodeGen.cpp
765

Nit: Should we have a notify failure for the character case?

767

No change requested: Would using this same location for branches of individual cases be correct?

768

Nit: Should this be part of the verifier (also)?

mehdi_amini accepted this revision.Nov 10 2021, 6:08 PM
This revision is now accepted and ready to land.Nov 10 2021, 6:08 PM
clementval added inline comments.Nov 11 2021, 12:14 AM
flang/lib/Optimizer/CodeGen/CodeGen.cpp
767

Not sure what do mean here. We are using the same location right now.

LGTM. Accepting since others are Nit comments.

flang/lib/Optimizer/CodeGen/CodeGen.cpp
767

I meant that the individual cases (for e.g case 1, case 2 below) have different locations in the source file. So the comparisons corresponding to the cases should ideally use the location of the individual cases.

select case
case 1:
case 2:
end select
clementval marked 2 inline comments as done.Nov 11 2021, 5:04 AM
clementval added inline comments.
flang/lib/Optimizer/CodeGen/CodeGen.cpp
767

Ok I get it. Will check if I can update that easily

768

This is checked in the verifier in a slightly different way. I'll just drop it here.

clementval marked an inline comment as done.Nov 11 2021, 5:51 AM
clementval added inline comments.
flang/lib/Optimizer/CodeGen/CodeGen.cpp
767

I have tried to retrieved the loc from the case arg but it seems to be null. It's probably something taht will take more time to be fined tune.

flang/lib/Optimizer/CodeGen/CodeGen.cpp
767

OK. We can work on it a separate patch.

This revision was automatically updated to reflect the committed changes.