[PATCH] D45378: [InstCombine] Propagate null values from conditions to other basic blocks

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.

High-level question: what is this trying to do?
Can't you just use `ReplaceUsesOfWith()`, or something like that?



================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:2399
+      isCanonicalPredicate(Pred)) {
+    auto Cmp = dyn_cast<CmpInst>(BI.getOperand(0));
+    auto NullOp = Cmp->getOperand(0);
----------------
Just `cast<>`


================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:2400
+    auto Cmp = dyn_cast<CmpInst>(BI.getOperand(0));
+    auto NullOp = Cmp->getOperand(0);
+    auto B = TrueDest;
----------------
I thought it is written somewhere, but i can't find it in https://llvm.org/docs/ProgrammersManual.html
TLDR: don't use `auto` unless the type is already obvious (or it is an iterator/etc).

This should be something like
```
auto *Cmp = dyn_cast<CmpInst>(BI.getOperand(0));
???? *NullOp = Cmp->getOperand(0);
```


================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:2403
+    bool NullOpReplaced = false;
+    while (B) {
+      for (auto &I : *B) {
----------------
I'd expect this to be something closer to
```
for(??? *B = TrueDest; B != nullptr; B = B->getNextNode()) {

```


================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:2404
+    while (B) {
+      for (auto &I : *B) {
+        for (unsigned i = 0; i < I.getNumOperands(); ++i) {
----------------
Similarly, i have no clue what this is, 
```
for (Instr &I : *B) {
```
?


================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:2405
+      for (auto &I : *B) {
+        for (unsigned i = 0; i < I.getNumOperands(); ++i) {
+          auto Arg = I.getOperand(i);
----------------
I'd try to use range-based loop, if there is a function to go from the pointer to an index, or `setOperand()` would work with index.


================
Comment at: test/Transforms/InstCombine/cond-return-null.ll:18
+; CHECK-NEXT:    ret i8* [[STOREMERGE]]
+;
+
----------------
Do you edit the check-lines after `llvm/utils/update_test_checks.py`?
Don't. (the first line included)


Repository:
  rL LLVM

https://reviews.llvm.org/D45378