[PATCH] D91649: [AA] Split up LocationSize::unknown()
Alina Sbirlea via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Nov 17 16:38:57 PST 2020
asbirlea added a comment.
- `LocationSize::unknownMaybeNegative()` seems somewhat ambiguous in isolation; unless I see the other name or have the context from this patch. I'm either keep `LocationSize::unknown` or make it even more verbose `LocationSize::unknownPositiveOrNegative()`. I think the most restrictive case ( `LocationSize::unknownMaybeNegative()` or renamed version) should be the default `~uint64_t(0)`, but don't feel strongly about it.
- clang-format
================
Comment at: llvm/include/llvm/Analysis/MemoryLocation.h:272
+ LocationSize Size =
+ LocationSize::unknownNonNegative(),
const AAMDNodes &AATags = AAMDNodes())
----------------
Default to the most generic? `unknownMaybeNegative`
================
Comment at: llvm/lib/Analysis/BasicAliasAnalysis.cpp:1386
// stripped a gep with negative index ('gep <ptr>, -1, ...).
- if (V1Size != LocationSize::unknown() &&
- V2Size != LocationSize::unknown()) {
+ // TODO: The V2Size.hasValue() check is unnecessary.
+ if (V1Size.hasValue() && V2Size.hasValue()) {
----------------
Could you clarify the TODO?
================
Comment at: llvm/lib/Analysis/BasicAliasAnalysis.cpp:1637
+ // TODO: Relax GEP checks above and use unknownMaybeNegative().
+ PNSize = LocationSize::unknownNonNegative();
----------------
This is not `unknownMaybeNegative()` because of the above check `!cast<ConstantInt>(PVGEP->idx_begin())->isNegative()` ?
I would set this to `unknownMaybeNegative()` and leave the TODO.
================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:521
Value *Ptr = const_cast<Value*>(Loc.Ptr);
- AST.add(Ptr, LocationSize::unknown(), Loc.AATags);
+ AST.add(Ptr, LocationSize::unknownNonNegative(), Loc.AATags);
Accesses.insert(MemAccessInfo(Ptr, false));
----------------
For checks in loops using AST, I'd use `LocationSize::unknownMaybeNegative()`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D91649/new/
https://reviews.llvm.org/D91649
More information about the llvm-commits
mailing list