Skip to content

Commit

Permalink
Re-apply "[AArch64] Enable "sink-and-fold" in MachineSink by default (#…
Browse files Browse the repository at this point in the history
…67432)"

This re-applies commit ace20e2, which was reverted in eff4ef2.

The issues were fixed in:

  * b30765c [AArch64] Fix an incorrect handling of debug values in
    MachineSink (#68107)

  * b454b04 [AArch64] Fix a compiler crash in MachineSink (#67705)
  • Loading branch information
momchil-velikov committed Oct 6, 2023
1 parent f74aaca commit a9d0ab2
Show file tree
Hide file tree
Showing 2 changed files with 2 additions and 2 deletions.
2 changes: 1 addition & 1 deletion llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ static cl::opt<bool> EnableGISelLoadStoreOptPostLegal(
static cl::opt<bool>
EnableSinkFold("aarch64-enable-sink-fold",
cl::desc("Enable sinking and folding of instruction copies"),
cl::init(false), cl::Hidden);
cl::init(true), cl::Hidden);

extern "C" LLVM_EXTERNAL_VISIBILITY void LLVMInitializeAArch64Target() {
// Register the target.
Expand Down
2 changes: 1 addition & 1 deletion llvm/test/CodeGen/AArch64/sink-and-fold.ll
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
; RUN: llc -aarch64-enable-sink-fold=true < %s | FileCheck %s
; RUN: llc < %s | FileCheck %s
target triple = "aarch64-linux"

declare i32 @use(...)
Expand Down

13 comments on commit a9d0ab2

@cmtice
Copy link
Contributor

@cmtice cmtice commented on a9d0ab2 Oct 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a heads up: This commit is breaking some of our testing. We are currently working on trying to get a reproducer for you. Would you maybe consider reverting this?

@cmtice
Copy link
Contributor

@cmtice cmtice commented on a9d0ab2 Oct 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also causing clang itself to crash on several of our tests (still working on getting reproducers).

@cmtice
Copy link
Contributor

@cmtice cmtice commented on a9d0ab2 Oct 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We;ve got a reproducer, where clang crashes in MachineSink with llc, and will be posting it to this commit soon. In the meantime I am going to revert this commit.

@alinas
Copy link
Contributor

@alinas alinas commented on a9d0ab2 Oct 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reproducer attached, crashes with: llc -O3 reduced_arm_rn.ll (mv reduced_arm_rn.ll.pdf reduced_arm_rn.ll)
reduced_arm_rn.ll.pdf

@davemgreen
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, It looks like there is a StoreInstrCache that needs to be kept up to date with the recreated store instructions. Thanks for the report.

@aemerson
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davemgreen I think I already fixed this in 7510f32 after I noticed it breaking GlobalISel's test suite. I decided to try a point fix instead of reverting the original change, feel free to revert/improve the fix.

@davemgreen
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah that looks similar to what I was looking at. Thanks for trying the fix. @momchil-velikov FYI, you likely already saw. I wasn't sure if HasStoreCache should be kept up-to-date at the same time?

@momchil-velikov
Copy link
Collaborator Author

@momchil-velikov momchil-velikov commented on a9d0ab2 Oct 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cmtice @alinas @aemerson Thanks!

@davemgreen

I wasn't sure if HasStoreCache should be kept up-to-date at the same time?

Does not seem so. HasStoreCache has entries that:

  • map to false if there's no store instruction in the range, so replacing a store with another one in the same place does not affect those entries
  • map to true, if some compile time limits are exceeded or the range contains an unanalysable instruction, again replacing a store with a functionally identical store does not affect those entries, either there's some other unanalyzable instruction or the new store is just as unanalyzable as the old one.

But we can invalidate the cache only for instructions that ->mayStore().

@momchil-velikov
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've got a couple of pull requests[1][2], after those I'm going to recommit (I just ran llvm-test-suite on AArch64 with -O3 -flto and I previously did RelWithDebInfo bootstrap, so it should be good :) )
[1] #68676
[2] #68677

@aemerson
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@momchil-velikov This seems to have caused miscompiles in the test suite with GlobalISel (-fglobal-isel) at -Os. I don't have a Linux machine to test on, but if you have an arm64 Mac you should be able to repro. Specifically this breaks sqlite3 (as well as some others). For sqlite3 I bisected this down to the unixGetTempname function. Could you revert if the fix for this isn't quick?

@aemerson
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reverted it in 1950507 to unblock our testing.

@momchil-velikov
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aemerson Thanks, looking at it.

@momchil-velikov
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed by #69235

Please sign in to comment.