[PATCH] D121897: Support phi operand in __builtin_object_size folder

[Nikita Popov via Phabricator via llvm-commits]

nikic accepted this revision.
nikic added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/lib/Analysis/MemoryBuiltins.cpp:823
+  case ObjectSizeOpts::Mode::Min:
+    return (getSizeWithOverflow(LHS).slt(getSizeWithOverflow(RHS))) ? LHS : RHS;
+  case ObjectSizeOpts::Mode::Max:
----------------
Just as a note, because this is pre-existing behavior of the select code: I find it somewhat odd that we just check for the smaller/larger Size-Offset here, but return the full (Size, Offset) pair. So the Exact/Min/Max only applies to the Size-Offset value, while the Size or Offset values individual might not be Exact/Min/Max. Looking at how this is used, it probably doesn't matter in the end, but it looks a bit suspicious.


================
Comment at: llvm/lib/Analysis/MemoryBuiltins.cpp:828
+    return (getSizeWithOverflow(LHS).eq(getSizeWithOverflow(RHS))) ? LHS
+                                                                   : unknown();
+  }
----------------
Needs a default llvm_unreachable to suppress warning (at least with GCC).


================
Comment at: llvm/test/Transforms/InstCombine/builtin-object-size-phi.ll:1
+; RUN: opt -O1 -S < %s | FileCheck %s
+
----------------
nikic wrote:
> This should run `-instcombine` only.
Okay, this was seriously confusing... The fold did not happen without `-instcombine`, even though `-instcombine` had no effect on the IR.

The reason is that InstCombine required TLI, and then LowerConstantIntrinsics had an optional TLI dependency, so the no-op InstCombine had an effect.

I've fixed this in https://github.com/llvm/llvm-project/commit/ab2284a6437bff8ba14d21cd6f9da927351dc17a by making TLI non-optional.

You should be able to just use `-lower-constant-intrinsics` now, and move this to the `LowerConstantIntrinsics` directory.


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

https://reviews.llvm.org/D121897