Skip to content

Conversation

@KevinLeeFM
Copy link

Justification for changes

  • FieldType.hhelemOf()
    • Dead code. Removed entirely.
    • Won't affect precision.
  • RemovePtrToInt.ccisConstantNoPtr
    • We can no longer determine if a pointer is constant. In that case, we have to reserve to returning false.
    • Precision loss can happen, though may be somewhat restored by looking at surrounding context for use of the pointer
  • DsaLibFuncInfo.ccgenerateSpec
    • Refactored to avoid using getPointerElementType
    • Won't affect precision.
  • DsaLocal.cc
    • computeGepOffset
      • Refactored argument name to directly receive the source element type from GEP rather than a pointer type
        • Remainder of the logic within method refactored to reflect this fact
        • The sole client call in BlockBuilderBase::visitGep has also been modified to reflect this change
      • Condition for encountering a pointer in source element type is asserted as false to catch programming error as it should never happen.
    • GlobalBuilder::init
      • Condition for managing function pointer is disabled since one cannot determine if the pointer belongs to a function.
      • Not sure what functionality is lost by disabling it since I don't understand what this block does.
      • Should be easy to infer from use/def though.
    • IntraBlockBuilder::isBytePtrTy
      • Method removed since this can no longer be determined from pointer type. Even if we recover pointee type info from inferring load/store instructions, the method's signature is outdated as it only accepts a type rather than a pointer. Better to just write a whole new method from scratch if we want to find out pointer type.
    • IntraBlockBuilder::visitStoreInst
      • Check for omnichar pointer is removed since we can no longer gleam any valuable information from StoreOp's pointer.
      • Might be able to acquire original type by recursively checking the instructions that created the value, or by checking for casts further down the line??? (No clear idea how to do this)
    • computeIndexedOffset
      • The condition for checking pointer type has been entirely deleted since it makes no sense to me for either of its two client calling methods (both visitExtractValueInst and visitInsertValueInst to visit an instruction that indexes an opaque pointer).
        • Now if a pointer type is passed in, it should fail an assertion.
    • IntraBlockBuilder::visitMemTransferInst + transfersNoPointers
      • The entire effort to avoid unification upon a memory transfer by trusting the memory type has been fully hamstrung as pointer types are now opaque. FIXME comments has been added. This seems unavoidable.
  • FieldType.cc
    • GetInnermostTypeImpl
      • The 2 cases for when a pointer is observed leads to a deadend
    • IsOmnipotentPtr
      • Can no longer determine. Conservatively return false.
  • RemovePtrToInt.cc
    • The rewrite pattern has been modified per the comment
    • Many use of getPointerElementType are avoided purely through refactor
    • m_ty field removed as it appears to be redundant afterwards
    • No precision should be lost at all
  • ShadowMem.cc
    • Many use of getPointerElementType are avoided purely through refactor
    • No precision should be lost at all

[](const llvm::Type *ty) {
return ty->isIntegerTy() || ty->isFloatingPointTy();
});
// TODO: Can infer pointer type by checking its uses. For now, not much
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this idea as a helpful utility. Given a ptr, traverse its uses and collect possible types. Committing to the found types is not sound in general, but often this should be correct. This is equivalent to relying on pointer types. Lets integrate this!

// FIXME: This creates a node for a global function pointer. Needed by
// vtable in C++
// Disabled for now since in LLVM 15 there is no way to determine if the pointer is a
// function. May be inferenced by checking if the pointer is ever loaded as a function.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. This might also be a constant pointer to function, in which case its value can be examined to determined that it is an address of a function.

}
}

static bool isBytePtrTy(const Type *ty) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

convert to determine by use

a gcd of the variable offset.
*/
std::pair<int64_t, uint64_t> computeGepOffset(Type *ptrTy,
std::pair<int64_t, uint64_t> computeGepOffset(Type *srcElementTy,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this function needs to be reviewed. I do not see the immediate correspondence. I would expect srcElementTy to be srcElemTy (and be named the same), but then it is different from Ty in the old version. In the new version they seem to be used interchangeably. I think something got lost in conversion

bool transfersNoPointers(MemTransferInst &MI, const DataLayout &DL) {
Value *srcPtr = MI.getSource();
auto *srcTy = srcPtr->getType()->getPointerElementType();
// FIXME: LLVM 15, Kevin: Since we can no longer access pointee types, it is
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs more work. at least a basic solution based on pointer uses at first. need to build some test cases to evaluate. this is important.

((sourceCell.getNode()->links().size() == 0 &&
hasNoPointerTy(I.getSource()->getType()->getPointerElementType())) ||
transfersNoPointers(I, m_dl))) {
// FIXME: LLVM 15, Kevin: The removal of the pointer type means we can no longer avoid unification upon a memory
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not good enough. need a solution that at least works for common rust examples

break;
}
// FIXME: We're at a dead-end when we encounter a pointer type.
if (currentTy->isPointerTy()) break;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm... this requires more thought. are there examples/tests where this is used?

we can assume that opaque pointer is always a pointer to i8 instead of giving up completely

auto *ci = mkShadowCall(B, c, m_memStoreFn,
{m_B->getInt32(getFieldId(c)),
m_B->CreateLoad(v->getType()->getPointerElementType(), v),
m_B->CreateLoad(m_Int32Ty, v),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is that correct to use Int32Ty here? We run on both 32bit and 64bit platforms, make sure that this is correct and is not architecture dependent

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants