Page MenuHomePhabricator

[RISCV] Fix RISCVInstrInfo::getInstSizeInBytes for atomics pseudos
ClosedPublic

Authored by jrtc27 on Apr 3 2020, 5:13 PM.

Details

Summary

Without these, the generic branch relaxation pass will underestimate the
range required for branches spanning these and we can end up with
"fixup value out of range" errors rather than relaxing the branches.
Some of the instructions in the expansion may end up being compressed
but exactly determining that is awkward, and these conservative values
should be safe, if slightly suboptimal in rare cases.

Diff Detail

Event Timeline

jrtc27 created this revision.Apr 3 2020, 5:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 3 2020, 5:13 PM
jrtc27 edited the summary of this revision. (Show Details)Apr 3 2020, 6:21 PM
jrtc27 updated this revision to Diff 254976.Apr 3 2020, 6:24 PM

Correct comment.

LGTM. I guess some may be difficult to test. CodeGen/RISCV/branch-relaxation.ll CodeGen/AArch64/branch-relax-bcc.ll ...

Thanks for the fix Jessica.
Looking at, say, doAtomicBinOpExpansion it's not immediately obvious why some of those values are correct. Can you please provide more info, ideally in the code?
Is there really no way to provide tests for these?

Bumping this patch - Looking at the output of the riscv-expand-pseudos pass I'd say these values are accurate. I'd say comments here, and in the functions which expand the instructions to ensure things don't get changed - seems to me like tests would be a little tricky so perhaps omitting that would be fine just to get this landed.

Bumping this patch - Looking at the output of the riscv-expand-pseudos pass I'd say these values are accurate. I'd say comments here, and in the functions which expand the instructions to ensure things don't get changed - seems to me like tests would be a little tricky so perhaps omitting that would be fine just to get this landed.

Ugh yes I never followed up on this properly. I started trying to write test but it seemed really fragile; the tests could maybe enforce they were _approximately_ correct but at the IR level there's just not enough control to know exactly how many bytes you're trying to create a branch for. Maybe MIR tests would work. If someone wants to pick this up and add tests by all means please do, but I don't have the time available to do that myself unfortunately.

luismarques accepted this revision.Jul 6 2020, 3:07 PM

Alright, let's get this landed! Please just add comments in the expansion functions, noting the need to update these values if the expansion templates ever change, as Lewis says.

This revision is now accepted and ready to land.Jul 6 2020, 3:07 PM
asb accepted this revision.Jul 8 2020, 10:25 PM

LGTM, +1 on adding a comment to the expansion functions noting the need to update getInstSizeInBytes. Thanks!

asb added a comment.Jul 14 2020, 10:56 PM

Hi @jrtc27 - it would be nice to get this in before LLVM 11 branches. Will you have time to add the extra comment and commit today?

This revision was automatically updated to reflect the committed changes.