[PATCH] D28546: LowerTypeTests: Implement importing of type identifiers.
Peter Collingbourne via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Jan 20 13:17:48 PST 2017
pcc marked an inline comment as done.
pcc added inline comments.
================
Comment at: llvm/lib/Transforms/IPO/LowerTypeTests.cpp:735
+LowerTypeTestsModule::TypeIdLowering
+LowerTypeTestsModule::importTypeId(StringRef TypeId) {
+ TypeTestResolution &TTRes = Summary->getTypeIdSummary(TypeId).TTRes;
----------------
eugenis wrote:
> 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.
>
>
No, we don't need it. Removed in r292647.
================
Comment at: llvm/lib/Transforms/IPO/LowerTypeTests.cpp:746
+ auto *GV = dyn_cast<GlobalVariable>(C);
+ if (!GV || GV->getVisibility() == GlobalValue::HiddenVisibility)
+ return C;
----------------
eugenis wrote:
> I don't get this condition. Why would this variable exist and be non-hidden?
If `getOrInsertGlobal` created the global, it would have default visibility. Added a comment.
================
Comment at: llvm/lib/Transforms/IPO/LowerTypeTests.cpp:758
+ if (AbsWidth == PtrWidth)
+ SetAbsRange(~0ull, ~0ull);
+ else if (AbsWidth)
----------------
eugenis wrote:
> should this say 0ull, ~0ull ?
"~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.
https://reviews.llvm.org/D28546
More information about the llvm-commits
mailing list