[PATCH] D108961: [RISCV] MC relaxation for out-of-range conditional branch.

Mon Jan 30 11:01:45 PST 2023

asb added a comment.

In D108961#4090884 <https://reviews.llvm.org/D108961#4090884>, @reames wrote:

> As @craig.topper mentioned above, I stumbled into this issue last week when debugging an LTO failure.  The exact issue I hit was because BranchRelaxation doesn't account for the padding required in RISCV's alignment directive.  (Which is different from every other target.)  We could, of course, patch the particular issue in BranchRelaxation, but I want to make an argument for reversing course on this patch as well.  If we'd landed this patch, my hard to debug LTO correctness issue would have been a minor code quality issue at worst, and I'd not have lost a week of my life.  :)

I'm very sorry for that, I know from personal experience how galling it is to spend a long time debugging something to later find a patch that fixed it had already been proposed. I don't personally have a reliable system for tracking patches that don't get re-pinged by their authors, but there was a bug report for this issue here, so definitely a failure there...

> There was an argument made on thread previously that this patch adds rarely executed behavior which is likely to bit rot.  I think this argument gets things completely backwards.  The fundamental case requiring relaxation has to be implemented somewhere, and the assembler implementation is actually the easier to test of the alternatives.  The assembler change has dramatically less "blast radius" than the compiler BranchRelaxation approach.

I wanted to clarify this point - my concern was adding a rarely executed and insufficiently tested code path (which has in the past also been the source of hard to debug issues).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108961/new/

https://reviews.llvm.org/D108961