Page MenuHomePhabricator

[openmp] Mark used variables as retain as well
ClosedPublic

Authored by JonChesterfield on Jan 13 2022, 4:44 AM.

Details

Summary

D97446 changed the behaviour of 'used'. Compensate.

Diff Detail

Event Timeline

JonChesterfield requested review of this revision.Jan 13 2022, 4:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 13 2022, 4:44 AM

@ronlieb @gregrodgers I think this will fix the blocker for aomp if cherry-picked

ronlieb accepted this revision.Jan 13 2022, 5:55 AM

LGTM,

This revision is now accepted and ready to land.Jan 13 2022, 5:55 AM

I'm hopeful that D97446 can be adjusted so that attribute((used)) works out of the box, at which point we can consider reverting this. Until then, workaround seems reasonable.

This revision was automatically updated to reflect the committed changes.
jdoerfert reopened this revision.Jan 13 2022, 8:50 AM

This might have broke nvidia and I'm not sure I understand the logic.

Why do we need retain? When is the variable being removed? I have seen problems w/ AMDGPU offloading when globals are hidden and then not exposed, is that the same problem?

This revision is now accepted and ready to land.Jan 13 2022, 8:50 AM
JonChesterfield added a comment.EditedJan 13 2022, 8:53 AM

See D97446. Used no longer means "used by something so don't delete it".

I don't know why we had no multiply defined symbol errors with the old version, should have done. Maybe lucky interleaving of the breaking change to the attribute with the changes to the driver.

Tests in trunk don't use the environment variable, so didn't notice it missing / containing zero. In this case it was 'used' being turned into compiler.used then internalised, so not quite the same as symbol visibility. Similar sort of thing though

See D97446. Used no longer means "used by something so don't delete it".

I don't know why we had no multiply defined symbol errors with the old version, should have done. Maybe lucky interleaving of the breaking change to the attribute with the changes to the driver.

Tests in trunk don't use the environment variable, so didn't notice it missing / containing zero.

That doesn't help me. When is the variable be deleted?

See D97446. Used no longer means "used by something so don't delete it".

I don't know why we had no multiply defined symbol errors with the old version, should have done. Maybe lucky interleaving of the breaking change to the attribute with the changes to the driver.

Tests in trunk don't use the environment variable, so didn't notice it missing / containing zero. In this case it was 'used' being turned into compiler.used then internalised, so not quite the same as symbol visibility. Similar sort of thing though

I am afraid there is a confusion.

__attribute__((used)) did lower to llvm.used on ELF, but GNU ld/gold/ld.lld were always free to discard the section. Mach-O ld64 retains the section, though.

__attribute__((used)) lowers to llvm.compiler.used now, which has the same behavior as the old ELF llvm.used.

It is possible that some optimization passes do not respect llvm.compiler.used as Clang previously did not often emit llvm.compiler.used.

The section discard behaviour is opt-in though, so any program that wrote 'used' to retain the variable and then used gc-sections, discovered it didn't work, and worked around that. Probably by compromising on gc.

So we aren't going from a state of broken to better, we're going from a system that worked reliably for years (provided the linker wasn't told to deadstrip sections and the compiler wasn't told to put the global in it's own section) to one that silently discards symbols.

If the idea is llvm.used and llvm.compiler.used shall now have the same semantics, we can kill a long tail of bugs by deleting one of them.

The section discard behaviour is opt-in though, so any program that wrote 'used' to retain the variable and then used gc-sections, discovered it didn't work, and worked around that. Probably by compromising on gc.

So we aren't going from a state of broken to better, we're going from a system that worked reliably for years (provided the linker wasn't told to deadstrip sections and the compiler wasn't told to put the global in it's own section) to one that silently discards symbols.

If the idea is llvm.used and llvm.compiler.used shall now have the same semantics, we can kill a long tail of bugs by deleting one of them.

I am afraid that you may still have misunderstanding.

The patch series D97446 has moved ELF from a state of broken to better. It matches GCC. It is just different from traditional Mach-O Clang behavior.

The issue that some optimization passes may have problems is not an argument for not doing D97446.

I have mentioned on D97446 how the split of llvm.used and llvm.compiler.used is useful. I managed to 10+ object size decrease for some instrumentations.