This is an archive of the discontinued LLVM Phabricator instance.

[flang][fir][NFC] Move remaining types to TableGen type definition
ClosedPublic

Authored by clementval on Feb 18 2021, 12:23 PM.

Details

Summary

Move the remaing of FIR types to TableGen type definition. This follow suggestion in D96422.

Diff Detail

Event Timeline

clementval requested review of this revision.Feb 18 2021, 12:23 PM
clementval created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 18 2021, 12:23 PM
rriddle added inline comments.Feb 18 2021, 1:45 PM
flang/include/flang/Optimizer/Dialect/FIRTypes.td
355

I'd prefer that the printers for these are defined with the parser. Can you move these to C++ as well?

361

Remove the extra newline here.

374

You don't need to define a parser/printer for types with no parameters.

414–416
500
clementval marked 5 inline comments as done.

Address review comment

Remove useless change

flang/include/flang/Optimizer/Dialect/FIRTypes.td
355

Sure

500

Updated.

Harbormaster completed remote builds in B89838: Diff 324837.
jeanPerier accepted this revision.Feb 19 2021, 9:36 AM

Thanks for the refactoring. Looks good to me.

This revision is now accepted and ready to land.Feb 19 2021, 9:36 AM

Sync with fir-dev

Do the parsers/printers here have any test coverage?

flang/include/flang/Optimizer/Dialect/FIRTypes.td
86

You can use CArg to provided a default value for the map param.

505

Renaming the parameter to elementType would remove the need for this.

flang/lib/Optimizer/Dialect/FIRType.cpp
286

You should be able to do: isa<BoxType, BoxCharType, ...>()

309

nit: You should be able to just stream this in: printer << ", " << map;

316

This error message should be redundant, an error is already emitted for most of these cases. This seems to point to a lack of test coverage. We also try to avoid double error messages in general.

483–484

i = rows? The initialization here looks really weird,

clementval marked 6 inline comments as done.

Address review comment

Do the parsers/printers here have any test coverage?

Round trip tests in flang/test/Fir/fir-types.fir. I'm gonna upstream some "invalid" tests in a follow up patch as well to check the diagnostics.

flang/include/flang/Optimizer/Dialect/FIRTypes.td
505

It's not needed anymore so I just removed it.

flang/lib/Optimizer/Dialect/FIRType.cpp
286

Right. It's not new code so I didn't double checked it. Works fine with your suggestion. Thx!

483–484

It's also a code that was just moved around. I updated the initialization in the new patch version.

@rriddle Are you happy with the latest changes?

rriddle accepted this revision.Feb 22 2021, 2:45 PM

(sorry, didn't get a notification for this)

LGTM given that this is mostly moving around existing code.

flang/lib/Optimizer/Dialect/FIRType.cpp
238–241

static ?

758

Can you use a merged isa here?

849

Same here and in the rest of the CL.

clementval marked 3 inline comments as done.

Rebase + address review comment

Sync with fir-dev

schweitz accepted this revision.Feb 24 2021, 12:09 PM
This revision was landed with ongoing or failed builds.Feb 24 2021, 5:23 PM
This revision was automatically updated to reflect the committed changes.