[PATCH] D128877: [LoopCacheAnalysis] Fix a type mismatch bug in cost calculation

[Congzhe Cao via Phabricator via llvm-commits]

congzhe added a comment.

Hi folks, my apologies for the delay, and thanks for the input. I do agree with you that we need to treat this scev expansion more carefully, although for some places where `getNoopOrAnyExtend()` is called (like the piece of code that Bjorn posted above), both `Stride` and `TripCount` are positive so it might be straightforward to use `SE.getNoopOrZeroExtend()`, or maybe just `SE.getNoopOrAnyExtend()` as how it is used now.

Motivated by @Meinersbur Michael's comment that we might just do `zext` for the test case in this patch, looking into the function `isConsecutive()`, I'm thinking we can change this patch to the following code in order to handle the expansions more carefully. What do you think?

  const SCEV *Coeff = getLastCoefficient();
  const SCEV *ElemSize = Sizes.back();
  Type *WiderType = SE.getWiderType(Coeff->getType(), ElemSize->getType());
  Stride = SE.getMulExpr(SE.isKnownNegative(Coeff)
                             ? SE.getNoopOrSignExtend(Coeff, WiderType)
                             : SE.getNoopOrZeroExtend(Coeff, WiderType),
                         SE.isKnownNegative(ElemSize)
                             ? SE.getNoopOrSignExtend(ElemSize, WiderType)
                             : SE.getNoopOrZeroExtend(ElemSize, WiderType));

As a side note, in order not to block @bjope Bjorn's work, I'd like to mention that if we add a  target datalayout line to the IR that crashes (https://reviews.llvm.org/rGb941857b40edd7f3f3a9ec2ec85a26db24739774#1100674), it would work fine. Something like `target datalayout = "e-m:e-p:32:32-i64:64-v128:64:128-a:0:32-n32-S64"` is sufficient. I mentioned it in the summary of this patch but just in case if you missed it. Hope this could provide a workaround for now.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128877/new/

https://reviews.llvm.org/D128877