Page MenuHomePhabricator

LowerTypeTests: Implement importing of type identifiers.
ClosedPublic

Authored by pcc on Jan 10 2017, 6:31 PM.

Details

Summary

To import a type identifier we read the summary and create external
references to the symbols defined when exporting.

Event Timeline

pcc updated this revision to Diff 83909.Jan 10 2017, 6:31 PM
pcc retitled this revision from to LowerTypeTests: Implement importing of type identifiers..
pcc updated this object.
pcc added a reviewer: eugenis.
pcc added subscribers: llvm-commits, mehdi_amini.
eugenis added inline comments.Jan 20 2017, 11:44 AM
llvm/lib/Transforms/IPO/LowerTypeTests.cpp
735

This function does not import SizeM1BitWidth.
I realize it is unused, but this could be a surprise for someone later, comments in TypeIdLowering say that it should be set for ByteArray, Inline, AllOnes.
Do we even need this field? It looks like it could be reconstructed from SizeM1.

746

I don't get this condition. Why would this variable exist and be non-hidden?

758

should this say 0ull, ~0ull ?

pcc updated this revision to Diff 85176.Jan 20 2017, 1:17 PM
  • Address review comments
pcc marked an inline comment as done.Jan 20 2017, 1:17 PM
pcc added inline comments.
llvm/lib/Transforms/IPO/LowerTypeTests.cpp
735

No, we don't need it. Removed in r292647.

746

If getOrInsertGlobal created the global, it would have default visibility. Added a comment.

758

"~0ull, ~0ull" means the full set, whereas "0ull, ~0ull" means the full set except for the value ~0ull. From http://llvm.org/docs/LangRef.html#range-metadata : "The pair a,b represents the range [a,b).".

See also http://llvm-cs.pcc.me.uk/lib/IR/ConstantRange.cpp#237

Added a comment.

eugenis added inline comments.Jan 20 2017, 1:30 PM
llvm/lib/Transforms/IPO/LowerTypeTests.cpp
758

From http://llvm.org/docs/LangRef.html#range-metadata,

The range should not represent the full or empty set. That is, a!=b.

Does the documentation need to be updated?

pcc marked an inline comment as done.Jan 20 2017, 1:34 PM
pcc added inline comments.
llvm/lib/Transforms/IPO/LowerTypeTests.cpp
758

The doc for range is correct, but the one for absolute_symbol needs to be corrected. I'll do that.

eugenis accepted this revision.Jan 20 2017, 1:41 PM

LGTM

This revision is now accepted and ready to land.Jan 20 2017, 1:41 PM
This revision was automatically updated to reflect the committed changes.