[PATCH] D48323: Derive GEP index type from Data Layout (cont)

Mon Jun 25 01:52:01 PDT 2018

bjope added a comment.

In https://reviews.llvm.org/D48323#1141705, @delena wrote:

> In https://reviews.llvm.org/D48323#1137389, @bjope wrote:
>
> > AFAIK BasicAA assumes that all GEP indices have a common type. So "normalization" is for example needed before passes like GVN that uses MemoryDependenceResults, that is using BasicAA. Or maybe that is a bug in BasicAA?
> >
> > Passes like InstCombine might skip doing rewrites in some basic blocks (e.g. if they are unreachable from entry). So I do not think that we can't rely on InstCombine doing a normalization for all GEP:s in the function.
> >  So are we moving towards always using the "normalized" type from scratch (in all passes) when creating a GEP, or what is the plan here?
>
>
> I don't think that we are going towards the always-normalized GEP, not within these changes, at least. We say that when normalized, the type of index should not be always a pointer type. Each target may specify index type in the Data Layout.
>  This is about my patches.
>
> I don't know how is the canonization of the GEP form important for the alias analysis. It's more for loop transformations, SCEVs, GEP combining/simplifications, I think

I only know that once upon a time I made a change in SeparateConstOffsetFromGEP to avoid doing rewrites on unreachable code (thus the canonicalization that is done for GEP indices in that pass did not happen in unreachable code). After that we got asserts like this one:

  opt: ../lib/Support/APInt.cpp:193: llvm::APInt& llvm::APInt::operator+=(const llvm::APInt&): Assertion `BitWidth == RHS.BitWidth && "Bit widths must be the same"' failed.
  ...
  opt(llvm::BasicAAResult::GetLinearExpression(llvm::Value const*, llvm::APInt&, llvm::APInt&, unsigned int&, unsigned int&, llvm::DataLayout const&, unsigned int, llvm::AssumptionCache*, llvm::DominatorTree*, bool&, bool&) 0x357)[0x1650837]
  opt(llvm::BasicAAResult::GetLinearExpression(llvm::Value const*, llvm::APInt&, llvm::APInt&, unsigned int&, unsigned int&, llvm::DataLayout const&, unsigned int, llvm::AssumptionCache*, llvm::DominatorTree*, bool&, bool&) 0x345)[0x1650825]
  opt(llvm::BasicAAResult::GetLinearExpression(llvm::Value const*, llvm::APInt&, llvm::APInt&, unsigned int&, unsigned int&, llvm::DataLayout const&, unsigned int, llvm::AssumptionCache*, llvm::DominatorTree*, bool&, bool&) 0x345)[0x1650825]
  opt(llvm::BasicAAResult::GetLinearExpression(llvm::Value const*, llvm::APInt&, llvm::APInt&, unsigned int&, unsigned int&, llvm::DataLayout const&, unsigned int, llvm::AssumptionCache*, llvm::DominatorTree*, bool&, bool&) 0x345)[0x1650825]
  opt(llvm::BasicAAResult::GetLinearExpression(llvm::Value const*, llvm::APInt&, llvm::APInt&, unsigned int&, unsigned int&, llvm::DataLayout const&, unsigned int, llvm::AssumptionCache*, llvm::DominatorTree*, bool&, bool&) 0x345)[0x1650825]
  opt(llvm::BasicAAResult::GetLinearExpression(llvm::Value const*, llvm::APInt&, llvm::APInt&, unsigned int&, unsigned int&, llvm::DataLayout const&, unsigned int, llvm::AssumptionCache*, llvm::DominatorTree*, bool&, bool&) 0x345)[0x1650825]
  opt(llvm::BasicAAResult::constantOffsetHeuristic(llvm::SmallVectorImpl<llvm::BasicAAResult::VariableGEPIndex> const&, unsigned long, unsigned long, long, llvm::AssumptionCache*, llvm::DominatorTree*) 0x17e)[0x1650dde]
  opt(llvm::BasicAAResult::aliasGEP(llvm::GEPOperator const*, unsigned long, llvm::AAMDNodes const&, llvm::Value const*, unsigned long, llvm::AAMDNodes const&, llvm::Value const*, llvm::Value const*) 0x7d4)[0x16553a4]
  opt(llvm::BasicAAResult::aliasCheck(llvm::Value const*, unsigned long, llvm::AAMDNodes, llvm::Value const*, unsigned long, llvm::AAMDNodes, llvm::Value const*, llvm::Value const*) 0x49f)[0x1653c7f]
  opt(llvm::BasicAAResult::alias(llvm::MemoryLocation const&, llvm::MemoryLocation const&) 0x26a)[0x165431a]
  opt(llvm::AAResults::alias(llvm::MemoryLocation const&, llvm::MemoryLocation const&) 0x38)[0x1631a58]
  opt(llvm::MemoryDependenceResults::getSimplePointerDependencyFrom(llvm::MemoryLocation const&, bool, llvm::ilist_iterator<llvm::ilist_detail::node_options<llvm::Instruction, true, false, void>, false, false>, llvm::BasicBlock*, llvm::Instruction*, unsigned int*) 0x1fa)[0x174823a]
  opt(llvm::MemoryDependenceResults::getSimplePointerDependencyFrom(llvm::MemoryLocation const&, bool, llvm::ilist_iterator<llvm::ilist_detail::node_options<llvm::Instruction, true, false, void>, false, false>, llvm::BasicBlock*, llvm::Instruction*, unsigned int*) 0x57c)[0x17485bc]
  opt(llvm::MemoryDependenceResults::getPointerDependencyFrom(llvm::MemoryLocation const&, bool, llvm::ilist_iterator<llvm::ilist_detail::node_options<llvm::Instruction, true, false, void>, false, false>, llvm::BasicBlock*, llvm::Instruction*, unsigned int*) 0x8b)[0x1749b4b]
  opt(llvm::MemoryDependenceResults::GetNonLocalInfoForBlock(llvm::Instruction*, llvm::MemoryLocation const&, bool, llvm::BasicBlock*, std::vector<llvm::NonLocalDepEntry, std::allocator<llvm::NonLocalDepEntry> >*, unsigned int) 0xd9)[0x174a799]

I don't remember exactly why I thought that the assert above was connected to not having a canonical type for the GEP indices. Somehow I think this code was involved:

  bool BasicAAResult::constantOffsetHeuristic(
      const SmallVectorImpl<VariableGEPIndex> &VarIndices, LocationSize V1Size,
      LocationSize V2Size, int64_t BaseOffset, AssumptionCache *AC,
      DominatorTree *DT) {
    if (VarIndices.size() != 2 || V1Size == MemoryLocation::UnknownSize ||
        V2Size == MemoryLocation::UnknownSize)
      return false;

    const VariableGEPIndex &Var0 = VarIndices[0], &Var1 = VarIndices[1];

    if (Var0.ZExtBits != Var1.ZExtBits || Var0.SExtBits != Var1.SExtBits ||
        Var0.Scale != -Var1.Scale)
      return false;

    unsigned Width = Var1.V->getType()->getIntegerBitWidth();

    // We'll strip off the Extensions of Var0 and Var1 and do another round
    // of GetLinearExpression decomposition. In the example above, if Var0
    // is zext(%x + 1) we should get V1 == %x and V1Offset == 1.

    APInt V0Scale(Width, 0), V0Offset(Width, 0), V1Scale(Width, 0),
        V1Offset(Width, 0);
    bool NSW = true, NUW = true;
    unsigned V0ZExtBits = 0, V0SExtBits = 0, V1ZExtBits = 0, V1SExtBits = 0;
    const Value *V0 = GetLinearExpression(Var0.V, V0Scale, V0Offset, V0ZExtBits,
                                          V0SExtBits, DL, 0, AC, DT, NSW, NUW);
    NSW = true;
    NUW = true;
    const Value *V1 = GetLinearExpression(Var1.V, V1Scale, V1Offset, V1ZExtBits,
                                          V1SExtBits, DL, 0, AC, DT, NSW, NUW);

The same Width is used for both V0 and V1, but if I remember correctly I had Var1.V->getType()->getIntegerBitWidth() != Var0.V->getType()->getIntegerBitWidth() when the assert triggered. When letting SeparateConstOffsetFromGEP::canonicalizeArrayIndicesToPointerSize rewrite all GEPs again (even unreachable GEPs) I no longer got the assertion.

Repository:
  rL LLVM

https://reviews.llvm.org/D48323