rafael.espindola at gmail.com
Sun Mar 23 21:01:44 PDT 2014
On 17 March 2014 06:59, Vassil Vassilev <vasil.georgiev.vasilev at cern.ch> wrote:
> On 03/11/2014 01:05 PM, Rafael Espíndola wrote:
>>> I am not sure I understand the intended semantics of the UnnamedAddr
>>> argument to GetOrCreateLLVMGlobal. Is it “this use does not care about the
>>> specific address”? Because, if so, the (existing) logic is completely
>>> backwards: we need to be *clearing* the flag on the global if the argument
>>> is ever *false*. And that seems like the most reasonable semantics,
>>> Also, the idea in the existing code that all runtime variables are
>>> unnamed_addr seems bogus to me. __dso_handle is a specific example of a
>>> variable whose address value *is* semantically important. This sort of
>>> thing would be important if there were any plausible transformations at all
>>> that involve unnamed_addr on external variable declarations.
>>> Also, the assertion is wrong: if something introduces a “runtime"
>>> declaration (e.g. by just declaring and using a global variable with the
>>> right name) before CodeGen emits its first intrinsic use of it, the
>>> declaration will not be marked unnamed_addr.
>>> So, basically, right now, this entire code path is useless and wrong. Is
>>> there a plan to apply it in situations where it wouldn’t be useless, i.e. to
>>> non-runtime user declarations?
>> So, on how the patch is organized:
>> * The structure the patch uses is that when the variable is first
>> created, its unnamed_addr is set to the value of the argument.
>> * We then check that no user expect that a variable is unnamed_addr
>> when in fact it isn't. I tried to make the check more strict by
>> asserting that Entry->getUnnamedAddr() == UnnamedAddr, but that fails
>> deep down in some objc cases that create runtime variables and access
>> them as regular ones.
>> But to the more important point: Should we be adding unnamed_addr in
>> the first place? I am not sure. Back in
>> Chris asked if we should. I could not find the entire discussion (the
>> patch that ended up being committed is simpler) and I am still not
>> sure exactly when CreateRuntimeVariable should be used.
>> An alternative patch that simply never sets it in
>> CreateRuntimeVariable is attached.
More information about the cfe-commits