Page MenuHomePhabricator

AArch64: Fix invalid CCMP emission
ClosedPublic

Authored by MatzeB on Nov 5 2018, 7:22 PM.

Details

Summary

The code emitting AND-subtrees used to check whether any of the operands
was an OR in order to figure out if the result needs to be negated.
However the OR could be hidden in further subtrees and not immediately
visible.

Change the code so that canEmitConjunction() determines whether the
result of the generated subtree needs to be negated. Cleanup emission
logic to use this. I also changed the code a bit to make all negation
decisions early before we actually emit the subtrees.

This fixes http://llvm.org/PR39550

Diff Detail

Repository
rL LLVM

Event Timeline

MatzeB created this revision.Nov 5 2018, 7:22 PM
MatzeB added a comment.Nov 5 2018, 7:22 PM

Putting this up for early review, still need to create a minimal testcase.

MatzeB added a comment.Nov 5 2018, 7:39 PM

hmm this is still buggy...

MatzeB updated this revision to Diff 173070.Nov 7 2018, 4:09 PM

After pondering about this again, this should be the correct fix.

MatzeB added a comment.Nov 7 2018, 4:10 PM

@mstorsjo could you check if this fixes your original problem?

I think I'm gonna take the time to rework the explanatory comments a bit, since it even took myself the author quite a while to understand things again...

MatzeB updated this revision to Diff 173080.Nov 7 2018, 4:29 PM
  • Added two more tests
  • Revamped explanation of the transformation. It's better but I fear still very hard to crasp. Oh well it's as good as I can make it right now...
MatzeB updated this revision to Diff 173082.Nov 7 2018, 4:30 PM

fix typos

MatzeB updated this revision to Diff 173086.Nov 7 2018, 4:39 PM

Tweak some more comments.

MatzeB updated this revision to Diff 173087.Nov 7 2018, 4:40 PM

And fix one more variable name typo (sorry for all the updates).

MatzeB updated this revision to Diff 173088.Nov 7 2018, 4:41 PM

And fix one more variable name typo (sorry for all the updates).

I haven't tried to follow the exact detail of the change, but the overall change looks correct based on what I learnt from reproducing the issue originally, and I can confirm that it fixes my issue (both reduced and full original case). Thanks!

@ab Can you have a look on this one?

Or is @MatzeB confident enough in the change as is?

Also, should we try to get this, together with the preceding refactoring, into 7.0.1?

Maybe you can give it at least some cursory review?

Adding some more reviewers who were active in AArch64ISelLowering recently.

Or is @MatzeB confident enough in the change as is?
Also, should we try to get this, together with the preceding refactoring, into 7.0.1?

I feel confident that this code is bettter than before. And I would also recommend pushing it for 7.0.1 (if we had it on the buildbots for a couple days) considering that it fixes a miscompile in "normal" code.

test/CodeGen/AArch64/arm64-ccmp.ll
529 ↗(On Diff #173070)

Note those changes here and in the next test are benign, the result is the same either way (I even tested this with a program that tries all possible combinations of float numbers generating different comparison results).

Maybe you can give it at least some cursory review?

It LGTM from my cursory review, still leaving it open in case any of the other reviewers wants to pitch in, otherwise I'll mark it as approved early next week.

efriedma added inline comments.Nov 12 2018, 3:44 PM
lib/Target/AArch64/AArch64ISelLowering.cpp
1554 ↗(On Diff #173088)

Maybe include a second step here, which explicitly reassociates this expression?

1564 ↗(On Diff #173088)

Would it be possible to use an extra CCMP to "negate" a condition, using something like "ccmp xzr, xzr"? I'm not sure it's actually a good idea, but it seems feasible.

1609 ↗(On Diff #173088)

In other words, MustBeFirst is true when the outermost expression of the transformed tree is a "not", as opposed to an "and" or a "setcc"?

1611 ↗(On Diff #173088)

Missing documentation for WillNegate?

1642 ↗(On Diff #173088)

IsOR?

Maybe you can give it at least some cursory review?

Adding some more reviewers who were active in AArch64ISelLowering recently.

Or is @MatzeB confident enough in the change as is?
Also, should we try to get this, together with the preceding refactoring, into 7.0.1?

I feel confident that this code is bettter than before. And I would also recommend pushing it for 7.0.1 (if we had it on the buildbots for a couple days) considering that it fixes a miscompile in "normal" code.

@tstellar FYI, there's interest in getting this in for 7.0.1, but it's still pending @MatzeB's response to @efriedma's comments. But of course, I understand that if it doesn't make the merge request deadline, then it'll have to be skipped.

MatzeB updated this revision to Diff 176310.Dec 2 2018, 6:36 PM
MatzeB marked 6 inline comments as done.
  • Address review feedback
This revision is now accepted and ready to land.Dec 3 2018, 11:41 AM

@MatzeB Do you have time to commit this?

Closed by commit rL348444: AArch64: Fix invalid CCMP emission (authored by matze, committed by ). · Explain WhyDec 5 2018, 5:43 PM
This revision was automatically updated to reflect the committed changes.