[PATCH] D61144: [LoopIdiomRecognize] BCmp loop idiom recognition

Fri Aug 23 01:52:30 PDT 2019

lebedev.ri added inline comments.

================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1986
+
+  BCmpInst = cast<ICmpInst>(BCmpValue);        // FIXME: is there no
+  LatchCmpInst = cast<CmpInst>(LatchCmpValue); // way to combine
----------------
courbet wrote:
> If you refactor as mentioned above, you can do the casts in the subfunctions. That being said I don't think the framework allows the cast during the match, which would be cool.
Indeed, you can't cast during patternmatch, this seems like the best place for this.

================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:2129
+  // we'd want to compare more bytes than could be represented by size_t, But
+  // allocation functions also take size_t. So how'd you produce such buffer?
+  NBytes = SE->getMulExpr(
----------------
courbet wrote:
> lebedev.ri wrote:
> > nikic wrote:
> > > Is this kind reasoning legal on the level of LLVM IR? Especially if no inbounds GEPs are involved, aren't we just dealing in raw memory and there isn't necessary any object with representable size involved?
> > > 
> > > (Context: Wondering about this in https://reviews.llvm.org/D61934#inline-559489 and your comment seems like a plausible explanation.)
> > I don't have an answer.
> > The obvious alternative would be to do saturating multiplication, i guess?
> Saturating multiplication would actually change the semantics of the code. The memset loop idiom already has this issue actually.
> I don't think the explanation is convincing because we could indeed be loading from raw memory. I guess the only thing that is saving us right now is that it's unlikely that people are memset/memcmp-ing more than size_t-max bytes of memory in real life. But the optimization is buggy :(
Added FIXME for now.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61144