This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Fix Chain/Glue Bug in PerformVMOVhrCombine
ClosedPublic

Authored by lenary on Feb 10 2023, 1:44 AM.

Details

Summary

In this optimisation, the Chain and Glue from the original CopyFromReg
was being lost by this optimisation, which resulted in miscompiles.

This fix just ensures that the input chains are correctly updated, and
that any any users are also updated with the new chain from the new
CopyFromReg.

Fixes #60510.

Diff Detail

Event Timeline

lenary created this revision.Feb 10 2023, 1:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 10 2023, 1:44 AM
lenary requested review of this revision.Feb 10 2023, 1:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 10 2023, 1:44 AM

Sounds OK if you can clear up the tests. It might be worth making sure there is a test with multiple uses too.

llvm/lib/Target/ARM/ARMISelLowering.cpp
15005

Can remove the extra t18: f16 = ARMISD::VMOVhr t5?

Sounds OK if you can clear up the tests. It might be worth making sure there is a test with multiple uses too.

Do you mean the AES pass tests? Do you think that should happen before we land this, they've been an issue for a little while.

llvm/lib/Target/ARM/ARMISelLowering.cpp
15005

Ack

Do you mean the AES pass tests? Do you think that should happen before we land this, they've been an issue for a little while.

I think I just meant https://reviews.llvm.org/D143712#4117617 for "clear up the tests". A Multiple uses test would be useful to check that nothing odd goes on when the returned value is used elsewhere too.

lenary added a comment.EditedFeb 20 2023, 9:32 AM

Do you mean the AES pass tests? Do you think that should happen before we land this, they've been an issue for a little while.

I think I just meant https://reviews.llvm.org/D143712#4117617 for "clear up the tests". A Multiple uses test would be useful to check that nothing odd goes on when the returned value is used elsewhere too.

Ah, right. Sorry, that leaked out my memory. Yes, I do intend to do a multiple uses test as well.

lenary updated this revision to Diff 501854.Mar 2 2023, 6:57 AM
lenary edited the summary of this revision. (Show Details)
lenary added a comment.Mar 2 2023, 7:00 AM

Tests and comment updated. I also split this patch stack away from the unrelated changes for inline asm handling of fp16/bf16.

dmgreen accepted this revision.Mar 2 2023, 2:05 PM

Thanks. LGTM

This revision is now accepted and ready to land.Mar 2 2023, 2:05 PM
This revision was landed with ongoing or failed builds.Mar 6 2023, 3:57 AM
This revision was automatically updated to reflect the committed changes.