[PATCH] D90610: [Inline] Fix in handling of ptrtoint in InlineCost

[Mikael Holmén via Phabricator via llvm-commits]

uabelho created this revision.
uabelho added reviewers: spatel, mtrofin.
Herald added subscribers: bjope, haicheng, hiraditya, eraman.
Herald added a project: LLVM.
uabelho requested review of this revision.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D90610

Files:
  llvm/lib/Analysis/InlineCost.cpp
  llvm/test/Transforms/Inline/inline-ptrtoint-different-sizes.ll


Index: llvm/test/Transforms/Inline/inline-ptrtoint-different-sizes.ll
===================================================================
--- /dev/null
+++ llvm/test/Transforms/Inline/inline-ptrtoint-different-sizes.ll
@@ -0,0 +1,40 @@
+; 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
Index: llvm/lib/Analysis/InlineCost.cpp
===================================================================
--- llvm/lib/Analysis/InlineCost.cpp
+++ llvm/lib/Analysis/InlineCost.cpp
@@ -1104,8 +1104,29 @@
   if (IntegerSize >= DL.getPointerSizeInBits(AS)) {
     std::pair<Value *, APInt> BaseAndOffset =
         ConstantOffsetPtrs.lookup(I.getOperand(0));
-    if (BaseAndOffset.first)
-      ConstantOffsetPtrs[&I] = BaseAndOffset;
+    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,


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D90610.302273.patch
Type: text/x-patch
Size: 2834 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20201102/00b37d18/attachment.bin>