[PATCH] D78765: [TRE] Fix bug in handling of switch statements

Eli Friedman via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Apr 24 17:22:09 PDT 2020


efriedma added a comment.

> However, all this switch handling really does is propagate a constant. Maybe we should remove it entirely and expect that the constant propagation passes do this for us.

This sort of depends on how the interaction with other passes works out.  A switch with one case will be turned into a branch by SimplifyCFG, so it's not worth optimizing.  If it's realistic to have a switch where only one of the cases goes to a return instruction, maybe worth keeping the switch handling around; SimplifyCFG will often prefer a unified "ret" instruction.  Probably want to check what the input to -tailcallelim looks like, realistically.

------

In terms of whether we could actually transform my original example... well, it sort of goes back to what I was saying on D78259 <https://reviews.llvm.org/D78259>.  Currently, we set the initial value of the "accumulator" to the value returned in the base case.  So if the base case isn't a value we can materialize in the entry block, we can't do the tail call transform.  But I'm not sure that's a fundamental limitation.  Suppose, instead, the accumulator always starts at zero.  Then, just before we return, we add the value of the base case.  It's still basically the same transform, but it's more flexible: you can drop the whole isDynamicConstant() check, I think.  Does that make sense?

To write this out:

Input

  define i32 @f() local_unnamed_addr {
  entry:
    %call = call i32 @g()
    switch i32 %call, label %sw.default [
      i32 1, label %cleanup
      i32 2, label %cleanup
    ]
  
  sw.default:
    %call1 = call i32 @f()
    %add = add nsw i32 %call1, 1
    ret i32 %add
  
  cleanup:
    ret i32 %call
  }
  
  declare i32 @g()

Current, invalid -tailcallelim output on trunk:

  define i32 @f() local_unnamed_addr {
  entry:
    br label %tailrecurse
  
  tailrecurse:                                      ; preds = %sw.default, %entry
    %accumulator.tr = phi i32 [ %call, %entry ], [ %add, %sw.default ]
    %call = tail call i32 @g()
    switch i32 %call, label %sw.default [
      i32 1, label %cleanup
      i32 2, label %cleanup
    ]
  
  sw.default:                                       ; preds = %tailrecurse
    %add = add nsw i32 %accumulator.tr, 1
    br label %tailrecurse
  
  cleanup:                                          ; preds = %tailrecurse, %tailrecurse
    ret i32 %accumulator.tr
  }
  
  declare i32 @g()

Alternative transform:

  define i32 @f() local_unnamed_addr {
  entry:
    br label %tailrecurse
  
  tailrecurse:                                      ; preds = %sw.default, %entry
    %accumulator.tr = phi i32 [ 0, %entry ], [ %add, %sw.default ]
    %call = tail call i32 @g()
    switch i32 %call, label %sw.default [
      i32 1, label %cleanup
      i32 2, label %cleanup
    ]
  
  sw.default:                                       ; preds = %tailrecurse
    %add = add nsw i32 %accumulator.tr, 1
    br label %tailrecurse
  
  cleanup:                                          ; preds = %tailrecurse, %tailrecurse
    %accumulator.ret = add i32 %accumulator.tr, %call
    ret i32 %accumulator.ret
  }
  
  declare i32 @g()



================
Comment at: llvm/test/Transforms/TailCallElim/accum_recursion.ll:44
-
-define i64 @test3_fib(i64 %n) nounwind readnone {
-; CHECK-LABEL: @test3_fib(
----------------
Please keep this function as a testcase, even if we're just keeping it an example of something we currently can't transform.


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

https://reviews.llvm.org/D78765





More information about the llvm-commits mailing list