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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

In D65280#1647026 <https://reviews.llvm.org/D65280#1647026>, @joerg wrote:

> Chandler, are you OK with getting the InstructionSimplify.h part in now, so that it can be merged into 9.0 and the rest follow separately?


Can we not get the entire thing merged? I'd really like that... I think the patch is actually really close. I have a bunch of comments below but they're all pretty boring in reality.



================
Comment at: include/llvm/Analysis/InstructionSimplify.h:265-266
 /// instruction must not be equal to the simplified value 'SimpleV'.
+/// If Failed is provided, instructions that could not be simplified are
+/// added to it.
 ///
----------------
I'd call this `UnsimplifiedUsers`.


================
Comment at: lib/Analysis/InstructionSimplify.cpp:5225-5226
 /// instructions to process and attempt to simplify it using
-/// InstructionSimplify.
+/// InstructionSimplify. Instructions that did not simplify are optionally added
+/// to the Failed set for further processing by the caller.
 ///
----------------
I'd make this a bit more precise...

"Recursively visited users which do not themselves simplify are ..."


================
Comment at: lib/Analysis/InstructionSimplify.cpp:5235
+                                              SmallSetVector<Instruction *, 8>
+                                               *Failed = nullptr) {
   bool Simplified = false;
----------------
If this is what `clang-format` did with this... I am sad. If not, clang-format?


================
Comment at: lib/Transforms/Scalar/LowerConstantIntrinsics.cpp:41
+
+#include <stdio.h>
+
----------------
No #include after usings and statistics and such please.


================
Comment at: lib/Transforms/Scalar/LowerConstantIntrinsics.cpp:45-51
+  Value *Result;
+
+  if (isa<Constant>(Op))
+    Result = ConstantInt::getTrue(II->getType());
+  else
+    Result = ConstantInt::getFalse(II->getType());
+  return Result;
----------------
Seems simpler to write as:

```
return isa<Constant>(Op) ? ConstantInt::getTrue(II->getType())
                         : ConstantInt::getFalse(II->getType());
``` 


================
Comment at: lib/Transforms/Scalar/LowerConstantIntrinsics.cpp:54-55
+
+static bool replaceWithSimplifyCFG(Instruction *II, Value *NewValue)
+{
+  bool HasDeadBlocks = false;
----------------
clang-format please.

I'd also call this `replaceConditionalBranchesOnConstant`.


================
Comment at: lib/Transforms/Scalar/LowerConstantIntrinsics.cpp:113-117
+        WeakTrackingVH InstPtr(&*I);
+        HasDeadBlocks = replaceWithSimplifyCFG(Inst, NewValue);
+        Changed = true;
+        if (!InstPtr || !I->getParent())
+          I = BB.begin();
----------------
As I tried to indicate before, this can be even simpler AFAICT.

I think you should keep a single worklist of unsimplified users. Then you should just do the recursive inst simplify here using the same worklist each time. You'll want to change the loop to go in RPO over the function rather than in the naive order.

Then below this loop you will have a single worklist of unsimplified users that you can walk with the exact code you have above AFAICT.


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

https://reviews.llvm.org/D65280