[PATCH] D134097: [NFC] Refactor creation of symbol+addend references

[Maksim Panchenko via Phabricator via llvm-commits]

maksfb accepted this revision.
maksfb added a comment.

Add "[BOLT]" to the title. Replace "this code" in the Summary with the description of the code. Otherwise LGTM.



================
Comment at: bolt/lib/Core/BinaryFunction.cpp:1054
+  (void)ReplaceSuccess;
+  assert(ReplaceSuccess && "failed to replace mem operand with symbol+off");
 }
----------------
rafauler wrote:
> yota9 wrote:
> > Maybe change assert to if + exit ?
> Are we migrating away from assertions? This is not supposed to fail at all, unless we modify our codebase in weird ways, so I would like to assert here. But I'm not sure how hard are people trying to get rid of assertions.
> 
> Another reason this might fail is if somebody is writing a new backend and fails to replace the memory operand of a given instruction that they need to support replacing.
It it's a validation of internal assumptions, not an assumption about the input, then it should be an assertion.


================
Comment at: bolt/lib/Core/BinaryFunction.cpp:1054
+  (void)ReplaceSuccess;
+  assert(ReplaceSuccess && "failed to replace mem operand with symbol+off");
 }
----------------
maksfb wrote:
> rafauler wrote:
> > yota9 wrote:
> > > Maybe change assert to if + exit ?
> > Are we migrating away from assertions? This is not supposed to fail at all, unless we modify our codebase in weird ways, so I would like to assert here. But I'm not sure how hard are people trying to get rid of assertions.
> > 
> > Another reason this might fail is if somebody is writing a new backend and fails to replace the memory operand of a given instruction that they need to support replacing.
> It it's a validation of internal assumptions, not an assumption about the input, then it should be an assertion.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134097