[Inline] Fix in handling of ptrtoint in InlineCost

[uabelho]

Summary

ConstantOffsetPtrs contains mappings from a Value to a base pointer and
an offset. The offset is typed and has a size, and at least when dealing
with ptrtoint, it could happen that we had a mapping from a ptrtoint
with type i32 to an offset with type i16. This could later cause
problems, showing up in PR 47969 and PR 38500.

In PR 47969 we ended up in an assert complaining that trunc i16 to i16
is invalid and in Pr 38500 that a cmp on an i32 and i16 value isn't
valid.

[uabelho]

llvm/lib/Analysis/InlineCost.cpp

1104

I suppose the simplest fix would just be to change the >= to == here, so we only save stuff in ConstantOffsetPtrs when the sizes match exactly?

1124

I don't know if sext is what we want to do here, of it that may lead to problems?

[RKSimon]

llvm/test/Transforms/Inline/inline-ptrtoint-different-sizes.ll

10

Rename test1 references to PR47969 to simplify tracking in the future

26

Rename test2 references to PR38500 to simplify tracking in the future

[mtrofin]

Thanks for looking into this!

Should places where we use ConstantOffsetPtrs to determine if instructions may be simplified, like CallAnalyzer::visitCmpInst, be also adjusted? Or does the ContantExpr::getICmp call take care of different-sized constant ints?

[spatel]

llvm/lib/Analysis/InlineCost.cpp

1124

Signed math is what I would expect based on:
http://llvm.org/docs/LangRef.html#getelementptr-instruction
"These integers are treated as signed values where relevant."
...and we do similar in GEPOperator::accumulateConstantOffset() for example.

[bjope]

llvm/lib/Analysis/InlineCost.cpp

1124

Not sure exactly how these ConstantOffsetPtrs are used, but doesn't it matter that ptrtoint is defined as doing a zext if the integer size is greater than the size of the pointer (base+offset)?

[spatel]

llvm/lib/Analysis/InlineCost.cpp

1124

Hmm...yes, I think you're right:
http://llvm.org/docs/LangRef.html#ptrtoint-to-instruction
So disregard my GEP comment. It would be great to manufacture an example of this behavior, but I'm not sure how to do that.

[uabelho]

Changed names of the testcase functions and changed to zext in visitPtrToInt.

[uabelho]

llvm/lib/Analysis/InlineCost.cpp

1124

Ok, so I changed to use zext.

I tried fiddling with testcases to try to find cases where I could see a difference between using sext and zext but I've failed.

[uabelho]

In D90610#2368489, @mtrofin wrote:

Thanks for looking into this!

Should places where we use ConstantOffsetPtrs to determine if instructions may be simplified, like CallAnalyzer::visitCmpInst, be also adjusted? Or does the ContantExpr::getICmp call take care of different-sized constant ints?

I don't know. When I debugged PR49890 I just realized that the size mismatch was introduced with the ptrtoint, so I tried doing something about that instead of trying to handle it at every use of ConstantOffsetPtrs. And then I realized that fix seems to have fixed PR38500 as well.

[bjope]

llvm/lib/Analysis/InlineCost.cpp

1124

Just to clarify, I'm not sure zext is any better (or more correct). My point was that if you have %x = ptrtoint %y an %y is %base + %offset, then zext(%base+%offset) isn't neccessarily the same as zext(%base) + zext(%offset) (neither the same as sext(%base) + sext(%offset)). Unless maybe if these base&offset pairs are guaranteed to be inbound or non-wrapping/non-overflowing or something? Maybe that is implied?

[uabelho]

llvm/lib/Analysis/InlineCost.cpp

1124

I have no idea myself. As I said I tried writing a couple of different variants of the added tests to see if sext/zext/changing at line 1104 made any difference, and I didn't manage to do that.

Changing "<=" to "==" at 1104 solves the problem too and I absolutely don't mind doing that if you all agree that is the best/safest fix.

[spatel]

If we want to restrict this patch to equal widths as the immediate and safe fix, that seems fine to me.

Note that there are questions about pointer math raised here:
D90637
http://lists.llvm.org/pipermail/llvm-dev/2020-November/146368.html
cc @nikic @nlopes @aqjune @qcolombet

[mtrofin]

In D90610#2370539, @uabelho wrote:

In D90610#2368489, @mtrofin wrote:

Thanks for looking into this!

Should places where we use ConstantOffsetPtrs to determine if instructions may be simplified, like CallAnalyzer::visitCmpInst, be also adjusted? Or does the ContantExpr::getICmp call take care of different-sized constant ints?

I don't know. When I debugged PR49890 I just realized that the size mismatch was introduced with the ptrtoint, so I tried doing something about that instead of trying to handle it at every use of ConstantOffsetPtrs. And then I realized that fix seems to have fixed PR38500 as well.

I think it would be good to understand that, as part of this patch, as changing that may have performance implications.

[spatel]

llvm/lib/Analysis/InlineCost.cpp

1124

We should proceed with that simplest fix ("==") to avoid crashing. Perf improvement can be a follow-up. IIUC, there is no danger of regressions with that change because it can't succeed currently.

(Note that D90708 clarified LangRef pointer math semantics, so that might help answer the questions about enhancing this to work correctly with different-sized offsets.)

[uabelho]

Do the easiest fix.

[uabelho]

llvm/lib/Analysis/InlineCost.cpp

1124

Great, I don't mind this at all so I just uploaded such a fix.

[uabelho]

In D90610#2376116, @mtrofin wrote:

In D90610#2370539, @uabelho wrote:

In D90610#2368489, @mtrofin wrote:

Thanks for looking into this!

Should places where we use ConstantOffsetPtrs to determine if instructions may be simplified, like CallAnalyzer::visitCmpInst, be also adjusted? Or does the ContantExpr::getICmp call take care of different-sized constant ints?

I don't know. When I debugged PR49890 I just realized that the size mismatch was introduced with the ptrtoint, so I tried doing something about that instead of trying to handle it at every use of ConstantOffsetPtrs. And then I realized that fix seems to have fixed PR38500 as well.

I think it would be good to understand that, as part of this patch, as changing that may have performance implications.

Maybe it is. I don't have time to really dig into that, so if that is a prerequisite to landing the fix then someone else is free to take over this.

[spatel]

LGTM

[uabelho]

In D90610#2405687, @spatel wrote:

LGTM

Thanks! I'll push during the day.

llvm/lib/Analysis/InlineCost.cpp

if (simplifyInstruction(I, [&](SmallVectorImpl<Constant *> &COps) {
         return ConstantExpr::getPtrToInt(COps[0], I.getType());
       }))
     return true;
 
   // Track base/offset pairs when converted to a plain integer provided the
   // integer is large enough to represent the pointer.
   unsigned IntegerSize = I.getType()->getScalarSizeInBits();
   unsigned AS = I.getOperand(0)->getType()->getPointerAddressSpace();
   if (IntegerSize >= DL.getPointerSizeInBits(AS)) {
     std::pair<Value *, APInt> BaseAndOffset =
         ConstantOffsetPtrs.lookup(I.getOperand(0));
-    if (BaseAndOffset.first)
+    if (BaseAndOffset.first) {
+      // If the sizes of the result type and the stored BaseAndOffset match
+      // exactly we can reuse that as is. Otherwise we need to create a new
+      // offset of the correct size and use that instead.
+      // Using an offset of the wrong size may lead to problems later on if we
+      // e.g. have
+      //   %i = ptrtoint i16* %p to i32
+      //   %s = sub i32 %i, %i
+      //   %t = trunc i32 %s to i16
+      // with the pointer size being 16, we might have stored BaseAndOffset
+      // for %p with offset size 16 bits. If we just reuse that 16 bit
+      // offset also for %i, %s will end up as i16 0 and then an assertion
+      // triggers when handling the trunc since a trunc from i16 to i16 isn't
+      // valid.
+      if (IntegerSize == BaseAndOffset.second.getBitWidth())
         ConstantOffsetPtrs[&I] = BaseAndOffset;
+      else {
+        APInt OffsetWithCorrectSize = BaseAndOffset.second.sext(IntegerSize);
+        std::pair<Value *, APInt> BaseAndOffsetWithCorrectSize =
+          {BaseAndOffset.first, OffsetWithCorrectSize};
+        ConstantOffsetPtrs[&I] = BaseAndOffsetWithCorrectSize;
+      }
+    }
   }
 
   // This is really weird. Technically, ptrtoint will disable SROA. However,
   // unless that ptrtoint is *used* somewhere in the live basic blocks after
   // inlining, it will be nuked, and SROA should proceed. All of the uses which
   // would block SROA would also block SROA if applied directly to a pointer,
   // and so we can just add the integer in here. The only places where SROA is
   // preserved either cannot fire on an integer, or won't in-and-of themselves

llvm/test/Transforms/Inline/inline-ptrtoint-different-sizes.ll

+; RUN: opt < %s -inline -S | FileCheck %s
+
+; InlineCost used to have problems with the ptrtoint, leading to
+; crashes when visiting the trunc in test1_arg and the icmp in
+; test2_arg.
+
+target datalayout = "p:16:16"
+target triple = "x86_64-unknown-linux-gnu"
+
+define void @test1_arg(i16* %p) {
+  %cast = ptrtoint i16* %p to i32
+  %sub = sub i32 %cast, %cast
+  %conv = trunc i32 %sub to i16
+  ret void
+}
+
+define void @test1(i16* %x) {
+  call void @test1_arg(i16* %x)
+  ret void
+}
+
+; CHECK-LABEL: @test1(i16* %x)
+; CHECK-NOT:     call
+; CHECK:         ret void
+
+define void @test2_arg(i16* %p) {
+  %cast = ptrtoint i16* %p to i32
+  %sub = sub i32 %cast, %cast
+  %cmp = icmp eq i32 %sub, 0
+  ret void
+}
+
+define void @test2(i16* %x) {
+  call void @test2_arg(i16* %x)
+  ret void
+}
+
+; CHECK-LABEL: @test2(i16* %x)
+; CHECK-NOT:     call
+; CHECK:         ret void