Page MenuHomePhabricator

[CodeGen] Postprocess PHI nodes for callbr
ClosedPublic

Authored by void on Aug 19 2020, 7:58 PM.

Details

Summary

When processing PHI nodes after a callbr, resolve the PHI nodes on
the default branch are inserted after the callbr (inserted after
INLINEASM_BR). The PHI node values on the indirect branches are
inserted before the INLINEASM_BR.

Diff Detail

Event Timeline

void created this revision.Aug 19 2020, 7:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 19 2020, 7:58 PM
void requested review of this revision.Aug 19 2020, 7:58 PM
nickdesaulniers added inline comments.
llvm/test/CodeGen/X86/callbr-asm-phi-nodes.ll
10

What was the input C program, was it precisely https://godbolt.org/z/zqK1K7 from https://bugs.llvm.org/show_bug.cgi?id=47242. If so, this mov into %eax looks problematic; it gets overwritten on both branches of the INLINEASM_BR, and looks like a dead store to me.

craig.topper added inline comments.Aug 20 2020, 11:03 AM
llvm/test/CodeGen/X86/callbr-asm-phi-nodes.ll
10

I think its there because the value of @var is an input to the inline asm. But the asm doesn't use it?

llvm/test/CodeGen/X86/callbr-asm-phi-nodes.ll
10

ah, I think removing thread_local from @var may remove this noise from the test case.

void updated this revision to Diff 286889.Aug 20 2020, 1:15 PM

Remove extraneous thread_local from global variable.

void marked 3 inline comments as done.Aug 20 2020, 1:15 PM
void added inline comments.
llvm/test/CodeGen/X86/callbr-asm-phi-nodes.ll
10

Yeah, it was identical to the GodBolt code. I removed the thread_local.

The commit message is confusing for this -- this isn't a correctness fix, but a minor optimization, right?

void marked an inline comment as done.Aug 20 2020, 3:28 PM

The commit message is confusing for this -- this isn't a correctness fix, but a minor optimization, right?

I'm not convinced it's just an optimization, but I could be swayed. It seems to me that the value coming into the PHI shouldn't rely upon the semantics of the asm block. So for instance would this be incorrect?

bb1:
  %eax = MOV 0
  INLINEASM_BR "mov 37, %eax"
  JMP return

return:
  PHI bb1, 0 ...

The commit message is confusing for this -- this isn't a correctness fix, but a minor optimization, right?

I'm not convinced it's just an optimization, but I could be swayed. It seems to me that the value coming into the PHI shouldn't rely upon the semantics of the asm block. So for instance would this be incorrect?

Inline assembly must declare what registers it clobbers, and the compiler can safely assume other registers are preserved across it.

bb1:
  %eax = MOV 0
  INLINEASM_BR "mov 37, %eax"
  JMP return

return:
  PHI bb1, 0 ...

So, in this example the input inline-asm is invalid, because it failed to declare that it clobbered eax.

void added a comment.Aug 20 2020, 6:19 PM

The commit message is confusing for this -- this isn't a correctness fix, but a minor optimization, right?

I'm not convinced it's just an optimization, but I could be swayed. It seems to me that the value coming into the PHI shouldn't rely upon the semantics of the asm block. So for instance would this be incorrect?

Inline assembly must declare what registers it clobbers, and the compiler can safely assume other registers are preserved across it.

bb1:
  %eax = MOV 0
  INLINEASM_BR "mov 37, %eax"
  JMP return

return:
  PHI bb1, 0 ...

So, in this example the input inline-asm is invalid, because it failed to declare that it clobbered eax.

Ah! That's right. I forgot about the clobber list. I'll change the description.

void edited the summary of this revision. (Show Details)Aug 20 2020, 6:20 PM
jyknight added inline comments.Aug 29 2020, 3:35 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
10007

This seems a bit confusing.

I think it would probably be clearer to make the body of this loop into a separate function which handles a single successor BasicBlock. Call that function directly on the default successor from visitCallBr, and move the loop over successors with the callbr default-successor-exclusion into visit().

10082

This is problematical if we're calling this function multiple times now. A constant will be emitted redundantly, if it's present both in indirect and default successor phi branches. Oddly enough, this is a member variable instead of a local, even though it's only used in this function. I think you can just move this call to clear() to the end of visit(), and that solves the problem.

The idea here is that we want to reduce the live-range of values used in the default destination? Makes sense as an optimization.

We could theoretically do this after isel, I think, but probably harder to implement.

This revision was not accepted when it landed; it landed in state Needs Review.Sep 24 2020, 2:34 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
void added a comment.Sep 24 2020, 2:35 PM

Erm...Reverted...Sorry.