[PATCH] D65280: Add a pass to lower is.constant and objectsize intrinsics

[Chandler Carruth via Phabricator via llvm-commits]

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

FWIW, the adjustments I'm suggesting around tightening the logic can easily be in a follow-up patch if you like. I think generally the code LGTM and I'd just like us to pin down exactly what changes we expect to happen w/ the handles as much as possible to avoid subtle latent bugs creeping in and never getting noticed.

The other two are trivial, feel free to land w/ those fixed.



================
Comment at: lib/Transforms/Scalar/LowerConstantIntrinsics.cpp:112-117
+    if (!II)
+      continue;
+    Value *NewValue;
+    switch (II->getIntrinsicID()) {
+    default:
+      continue;
----------------
joerg wrote:
> chandlerc wrote:
> > For both the `II` thing and the `default` case -- do we really expect these to ever fail?
> > 
> > I would expect either the VH to be null, or for it to definitively be one of the two intrinsics we added. Maybe switch to `cast_or_null` above with `VN.get()` or some such, and llvm_unreachable on the default case.
> Yes, the same concerns as with the earlier version still apply. The recursive simplification can change the instruction type in place or remove it. The logic is still simpler since no new instructions can appear.
I'm really surprised that it can *change* the value handle in this way.

I guess because we're using a tracking value handle (is that really necessary?) they may be moved onto the constant, but IMO that'd be more cleanly handled by checking for the value handle being either null or a non-instruction value. If its an instruction, it should really only be one of these two intrinsics or something deeply wrong has happened elsewhere, no?

I'm mostly suggesting we assert on that to track down the strange behavior and make sure the overall logic is actually still correct if it comes up rather than potentially hiding a deeper bug.


================
Comment at: test/Transforms/LowerConstantIntrinsics/crash-on-large-allocas.ll:1
-; RUN: opt -S -codegenprepare %s -o - | FileCheck %s
+; RUN: opt -S --lower-constant-intrinsics %s -o - | FileCheck %s
 ;
----------------
Probable just one `-` is fine?


================
Comment at: test/Transforms/LowerConstantIntrinsics/objectsize_basic.ll:1
-; RUN: opt -codegenprepare -S < %s | FileCheck %s
+; RUN: opt --lower-constant-intrinsics -S < %s | FileCheck %s
 
----------------
Probably just one `-` is fine?


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

https://reviews.llvm.org/D65280