erichkeane wrote: > Thanks for the elaboration! > > I agree that it indeed looks chaotic if we keep empty `[[no_unique_address]]` > members in CIR structs. And it could be horrible for LowerToLLVM authors to > keep track of the adjustments of GEP offsets. > > My concern here is that we now have two representations for constant data > member pointers, one based on the member index (`#cir.data_member`, already > present), and the other based on the offset (`#cir.data_member_offset` > introduced in this patch). And it's obvious that the former could be fully > replaced by the latter, i.e. we could just lower all constant data member > pointers to `#cir.data_member_offset` and then remove `#cir.data_member`. > Will this happen?
We COULD do this. I VERY much will fight anyone who tries to make that happen. I don't have a way of doing an assert here unfortunately, else I would. The member-index IS in fact the 'right' abstraction for us, if at all possible. So the `data_member_offset` is an escape hatch for cases where a field (basically ONLY one of these?) is not represented for good reason. IF we find anyone trying to use this for anything else, we HAVE to have a very intense discussion about it. > > Furthermore, I hope we could agree that "member index" is a higher-level > representation compared to "member offsets", and it's preferable to keep > higher-level information in CIR, at least in the early stages. If we're going > to unite `#cir.data_member` and `#cir.data_member_offset`, I hope we could > have an optional attribute in `#cir.data_member_offset` that gives the > indices of the referenced member. I don't intend/don't want us to change anything else lower in the stack. I think we should continue to use the member offsets whenever available. > Here are some long-term thoughts I have on this that may not be immediately > feasible in our current status (so I'm folding it in a `<detail>` block). > Details >Keeping empty members in CIR looks chaotic because the input source code >itself carries a ton of information. By removing these empty members we're >throwing away high-level information that could be valuable for CIR consumers. >So I think it's preferable to keep empty members and keep #cir.data_member >as-is. Yeah, I was on the fence with this, and I'd like to hear what @andykaylor thinks here too. I can cancel this patch and re-pick up my one that does this (though I'm not sure what I want to see this for). These no-unique-address members are pretty awkward, and taking an address of them is even weirder. You can't really DO anything with them, so my thoughts were "this is an ugly operation for a rarely/never used thing". >I'm actually expecting that in the long term, the ABI lowering pass (instead >of the LowerToLLVM pass) could: >Lower the high-level CIR struct (which contains empty members) to an ABI CIR >struct (which lacks empty members and aligns to LLVMIR struct). >Lower all #cir.data_member attributes to #cir.data_member_offset (or maybe >directly to #cir.int), by querying the layout information of the struct. I THINK we are already at the right abstraction TBH. We get our layouts correct through the LLVM MLIR pass, and the LLVM-MLIR keeps the member-offset. I personally think we should ALWAYS keep the member info/member offset around until we absolutely can't. Currently LLVM-MLIR does this, and I think that is the right abstraction point. https://github.com/llvm/llvm-project/pull/208777 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
