arsenm added a comment.

In D78862#2012560 <https://reviews.llvm.org/D78862#2012560>, @jdoerfert wrote:

> I think this is an improvement over the status quo and it looks fine to me.
>
> @arsenm I agree that we should tie this to the data layout (or at least 
> should try) but I guess there are open questions to answer and code to write.
>  I propose to accept this and work on the DL patch after. WDYT?


Seems ok, but it's still burning an enum value which I guess isn't super 
important. With the datalayout property, we might really want the inverse 
attribute

> 
> 
> In D78862#2008831 <https://reviews.llvm.org/D78862#2008831>, @manojgupta 
> wrote:
> 
>> @nikic Thanks for the work.
>>
>> In D78862#2003684 <https://reviews.llvm.org/D78862#2003684>, @arsenm wrote:
>>
>> > FWIW I think this attribute should be replaced with a data layout 
>> > property, so this would eventually be removed
>>
>>
>> @arsenm  Is there any work planned on moving to data layout? Moving to data 
>> layout may affect cross TU inlining e.g. LTO where 1 TU is compiled with 
>> `-fno-delete-null-pointer-checks` and other TU is not. There might be other 
>> potential impact that we might not know yet.
> 
> 
> If we link modules with mismatching data layouts we can/should deal with this 
> by utilizing more address spaces. That is, change the address space in one 
> module to a fresh one to keep the properties alive. There need to be rules 
> for this and infrastructure but something similar might be needed for 
> heterogeneous IR modules soon. Different story though. We can also combine 
> the attribute and the data layout if necessary, though I'm not a fan.

This sounds really problematic and would require way more knowledge of target 
address spaces. I don't think this will work


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78862/new/

https://reviews.llvm.org/D78862



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to