https://github.com/xlauko commented:
Not sure whether we want to tie in ABI specific decision to type itself.
These three fields are TU properties, not type properties, and belong at module
scope?
Also this might lead to weird behavior when usign same context for multiple TUs
with possibly different targes, maybe might happen when copiling with single
MLIR context for both CUDA and CPU (not sure whether one should do it but,
since present approach does not consided added fields in type uniquing it might
be a potential footgun).
Maybe cleaner solution would be to split these two?
I am thinking in a direction of module level attribute:
```
module attributes {
cir.record_layouts = {
"Foo" = #cir.record_layout<
can_pass_in_registers = true,
has_trivial_destructor = true,
align_in_bytes = 8
>,
"Bar" = #cir.record_layout<
can_pass_in_registers = false,
has_trivial_destructor = false,
align_in_bytes = 16
>
}
} { ... }
```
The motivation is that `canPassInRegisters`, `hasTrivialDestructor`, and
`recordAlignInBytes` aren't really properties of the type. They're properties
of how this translation unit chose to lay the type out under its target ABI.
This would result in:
- `RecordTypeStorage` to handle only the storegat that participates in
uniquing, and we won't store additional fields for anonymous structs as the
proposed solution. `mutate()` correspondingly loses three parameters and only
handles the recursive-completion case.
- Get roundtrip (printer/parser for attribute for free) as module attributes
won't be anyhow special.
- Make the "layout not yet computed" state explicit and checkable: a verifier
walks identified RecordTypes reachable from the module and asserts each has an
entry in cir.record_layouts. Today the equivalent check would have to inspect
three sentinel-ish booleans and decide whether they're "real" or
default-constructed, which isn't really checkable at all.
Implementation-wise I would suggest then:
- build a DenseMap<StringAttr, RecordLayoutAttr> during CIRGen, materialize the
DictionaryAttr once at the end of frontend lowering
- add a helper like`CIRDialect::getRecordLayout(ModuleOp, StringAttr)`.
- register a dialect-level verifier for cir.record_layouts via
`CIRDialect::verifyOperationAttribute` which for each identified, complete
record, assert there's a matching entry in the dictionary - for this it would
be nicer to SymbolTable for records, as now one needs to walk to module to
gather all record types
- I am not happy about this point, potentially we can just assert in
`getRecordLayout` and hope that noone forgets to populate module attribute, or
do the verification only once we go to llvm since we walk the module in that
case anyway, but then it might be harder to backtrack where the real error
happend?
https://github.com/llvm/llvm-project/pull/188300
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
