This is an archive of the discontinued LLVM Phabricator instance.

[VNCoercion] Disallow coercion between different ni addrspaces
ClosedPublic

Authored by vchuravy on Jul 30 2018, 2:09 PM.

Details

Summary

I'm not sure if it would be legal by the IR reference to introduce
an addrspacecast here, since the IR reference is a bit vague on
the exact semantics, but at least for our usage of it (and I
suspect for many other's usage) it is not. For us, addrspacecasts
between non-integral address spaces carry frontend information that the
optimizer cannot deduce afterwards in a generic way (though we
have frontend specific passes in our pipline that do propagate
these). In any case, I'm sure nobody is using it this way at
the moment, since it would have introduced inttoptrs, which
are definitely illegal.

Fixes PR38375

Co-authored-by: Keno Fischer <keno@alumni.harvard.edu>

Diff Detail

Event Timeline

loladiro created this revision.Jul 30 2018, 2:09 PM
vchuravy commandeered this revision.Jan 14 2020, 12:06 AM
vchuravy added a reviewer: loladiro.
Herald added a project: Restricted Project. · View Herald TranscriptJan 14 2020, 12:06 AM
vchuravy changed the repository for this revision from rL LLVM to rG LLVM Github Monorepo.Jan 14 2020, 12:07 AM

Unit tests: pass. 61801 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

vchuravy updated this revision to Diff 237874.Jan 14 2020, 1:10 AM

clang format

vchuravy edited the summary of this revision. (Show Details)Jan 14 2020, 1:11 AM
vchuravy added a project: Restricted Project.

Unit tests: pass. 61801 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Bump for review. We (Julia) have been carrying this one for a while.

reames accepted this revision.Dec 7 2020, 11:24 AM

LGTM.

As a side note, I don't think you actually need to disallow conversion of the null pointer between two different NI address spaces. If you wanted to improve opt quality, folding your check into the previous one would be beneficial. I'm not going to require that as this a) fixes a correctness bug, and b) has been outstanding for way to long.

This revision is now accepted and ready to land.Dec 7 2020, 11:24 AM

Thanks! I rebased the PR and landed it.

Also, even on Linux it seems Clang is inclined to link against libgcc rather than libclang by default (at least that's what my Debian one does). When did these functions get into libgcc? I'm worried that a significant proportion of users there will find themselves having to add extra command-line options (whether disabling this or forcing libclang) to produce binaries.

Sorry, ignore that. Wrong review.