Page MenuHomePhabricator

[mlir] don't drop undef initializers in translation to LLVM IR

Authored by ftynse on Jul 8 2021, 7:07 AM.



LLVM IR allows globals with external linkage to have initializers, including
undef. The translation was incorrectly using undef as a indicator that the
initializer should be ignored in translation, leading to the impossibility to
create an external global with an explicit undef initializer. Fix this and use
nullptr as a marker instead.

Diff Detail

Event Timeline

ftynse created this revision.Jul 8 2021, 7:07 AM
ftynse requested review of this revision.Jul 8 2021, 7:07 AM
wsmoses added inline comments.Jul 8 2021, 8:33 AM

This comment isn't 100% correct. My understanding of the LLVM semantics is that for an external linkage variable if you have an initializer the symbol is externally visible and defined within the module, if you do not have an initializer the symbol is defined in another translation unit and available here.

As such I don't think we should be assigning the initializer by default (since the explicit inclusion or omission of the initializer is what defines the semantics).


Per earlier comment I would also add a test which explicitly doesn't have an initializer in mlir and ensure it does not in llvm either.

wsmoses accepted this revision.Jul 8 2021, 2:02 PM
wsmoses added inline comments.

Ignore my previous comment about not assigning undef by default, rereading this it should only occur for non-external variables. The comment here is fine, but I may recommend adding to the comment for shouldDropGlobalInitializer with the previous explanation.


Ignore this comment, I'm seeing this behavior checked on line 78.

This revision is now accepted and ready to land.Jul 8 2021, 2:02 PM
ftynse marked 4 inline comments as done.Jul 9 2021, 8:52 AM

Added more comments before landing.