Move the remaing of FIR types to TableGen type definition. This follow suggestion in D96422.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| 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 | Use the builders field instead: https://mlir.llvm.org/docs/OpDefinitions/#custom-type-builder-methods | |
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 | ||
| 299 | You should be able to do: isa<BoxType, BoxCharType, ...>() | |
| 327 | 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. | |
| 327 | nit: You should be able to just stream this in: printer << ", " << map; | |
| 472–473 | i = rows? The initialization here looks really weird, | |
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 | ||
| 299 | Right. It's not new code so I didn't double checked it. Works fine with your suggestion. Thx! | |
| 472–473 | It's also a code that was just moved around. I updated the initialization in the new patch version. | |
You can use CArg to provided a default value for the map param.