[PATCH] D78259: [TailCallElim] Add handling of readonly functions

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

I'm concerned that hoisting instructions to the entry block isn't safe. A load can have undefined behavior, and you don't try to prove the load in question points to a valid address.  Is there some way to make this transform work without hoisting the operations to the entry block?  At first glance, it seems like it should be possible to instead sink the instructions into the return block.  But I haven't thought about it very carefully.



================
Comment at: llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp:391
       if (SI->getCondition() == V)
         return SI->getDefaultDest() != RI->getParent();
 
----------------
While you're in this area, the following currently crashes `opt -tailcallelim`:

```
define i32 @f() local_unnamed_addr {
entry:
  %call = call i32 @g()
  switch i32 %call, label %sw.default [
    i32 1, label %cleanup
    i32 5, label %cleanup
    i32 7, label %cleanup
  ]

sw.default:
  %call1 = call i32 @f()
  %add = add nsw i32 %call1, 1
  br label %cleanup

cleanup:
  %retval.0 = phi i32 [ %add, %sw.default ], [ %call, %entry ], [ %call, %entry ], [ %call, %entry ]
  ret i32 %retval.0
}

declare i32 @g()
```

Please take a look, since it seems related to what you're doing.


================
Comment at: llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp:400
+      bool OpsAreDynConsts = true;
+      for (Value *Op : I->operand_values()) {
+        OpsAreDynConsts = isDynamicConstant(Op, CI, RI);
----------------
llvm::all_of.


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

https://reviews.llvm.org/D78259