This is an archive of the discontinued LLVM Phabricator instance.

GlobalISel/AArch64: don't optimize away redundant branches at -O0
ClosedPublic

Authored by aprantl on Jun 30 2021, 3:48 PM.

Details

Summary

This patch prevents GlobalISel from optimizing out redundant branch instructions when compiling without optimizations.

The motivating example is code like the following common pattern in Swift, where users expect to be able to set a breakpoint on the early exit:

public func f(b: Bool) {
  guard b else { 
    return // I would like to set a breakpoint here.
  }
  ...
}

The patch modifies two places in GlobalISEL: The first one is in IRTranslator.cpp where the removal of redundant branches is made conditional on the optimization level. The second one is in AArch64InstructionSelector.cpp where an -O0 *only* optimization is being removed.

rdar://79515454

Diff Detail

Event Timeline

aprantl created this revision.Jun 30 2021, 3:48 PM
aprantl requested review of this revision.Jun 30 2021, 3:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2021, 3:48 PM

Sounds sensible to me. I wouldn't expect -O0 to be doing many optimizations.

llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
2095–2097

The formatting is off now.

llvm/test/DebugInfo/AArch64/fallthrough-branch.ll
5

This test looks like it could be cleaned up quite a bit. It doesn't need debug info for example, as far as I understand. Just a block that fallsthrough.

I think that prioritizing debuggability at -O0 is in general the right call, but I am a little curious about the impact on code size. Does this increase -O0 CTMark code size a lot?

I think that prioritizing debuggability at -O0 is in general the right call, but I am a little curious about the impact on code size. Does this increase -O0 CTMark code size a lot?

In fact it does, a lot. 8% geomean. However, SelectionDAG does the same thing so this patch would be making us match its behavior. It is unfortunate though.

@aprantl by the way, I'm pretty sure there are a fair number of tests that need updating with this change.

@aprantl by the way, I'm pretty sure there are a fair number of tests that need updating with this change.

Yes that' right:

Failed Tests (12):
  LLVM :: CodeGen/AArch64/GlobalISel/arm64-atomic.ll
  LLVM :: CodeGen/AArch64/GlobalISel/arm64-irtranslator.ll
  LLVM :: CodeGen/AArch64/unwind-preserved.ll
  LLVM :: CodeGen/BPF/BTF/func-func-ptr.ll
  LLVM :: CodeGen/BPF/BTF/func-non-void.ll
  LLVM :: CodeGen/BPF/BTF/func-typedef.ll
  LLVM :: CodeGen/BPF/BTF/func-unused-arg.ll
  LLVM :: CodeGen/BPF/BTF/func-void.ll
  LLVM :: CodeGen/Mips/GlobalISel/llvm-ir/jump_table_and_brjt.ll
  LLVM :: CodeGen/Mips/GlobalISel/llvm-ir/long_ambiguous_chain_s32.ll
  LLVM :: CodeGen/Mips/GlobalISel/llvm-ir/long_ambiguous_chain_s64.ll
  LLVM :: CodeGen/Mips/GlobalISel/llvm-ir/phi.ll

Before I go through updating all the tests, do we think that this is a reasonable change?

Pros:

  • Consistent with SelectionDAG behavior (fixes a "regression" in GlobalISEL)
  • Enables debugging scenarios (setting breakpoints on early returns, etc)

Cons:

  • Code size increase (-O0 only)

The patch improves debuggability, and debug builds are the primary reason why developers compile without optimizations. From my point of view this seems to be the right trade-off, but please let me know if there are any other interests we need to consider.

I agree that debug info quality is more important at -O0, so while 8% is a tough regression to take, I think it's the right thing to do here.

+1, debuggability should be the priority here.

aprantl updated this revision to Diff 356837.Jul 6 2021, 5:22 PM

Updated the rest of the codegen tests.

aprantl updated this revision to Diff 356839.Jul 6 2021, 5:23 PM

Upload correct patch...

aprantl updated this revision to Diff 356840.Jul 6 2021, 5:24 PM

And fixed a whitespace issue.

aemerson accepted this revision.Jul 6 2021, 11:31 PM

LGTM.

This revision is now accepted and ready to land.Jul 6 2021, 11:31 PM
This revision was landed with ongoing or failed builds.Jul 7 2021, 12:52 PM
This revision was automatically updated to reflect the committed changes.

This change appears to have broken LLDB testsuite failure https://lab.llvm.org/buildbot#builders/96/builds/9404

This change appears to have broken LLDB testsuite failure https://lab.llvm.org/buildbot#builders/96/builds/9404

Sounds like a familiar fragile test.

This broke LLDB buildbot testcase where breakpoint set at start of loop failed to hit.

https://lab.llvm.org/buildbot/#/builders/96/builds/9404

https://github.com/llvm/llvm-project/blob/main/lldb/test/API/commands/process/attach/main.cpp#L15

I have temporarily reverted this change.

This broke LLDB buildbot testcase where breakpoint set at start of loop failed to hit.

https://lab.llvm.org/buildbot/#/builders/96/builds/9404

https://github.com/llvm/llvm-project/blob/main/lldb/test/API/commands/process/attach/main.cpp#L15

I have temporarily reverted this change.

I agree the bot data seems to point to this commit. I'll try to figure out what happened there.

The LLDB test that was failing was badly written and was trying to set a breakpoint on a line that was not guaranteed to have any code associated with it. I reapplied the change, fixed the test and the bot green again.