[PATCH] D144189: [AIX][CodeGen] Storage Locations for Constant Pointers

Wed Mar 15 08:46:21 PDT 2023

hubert.reinterpretcast accepted this revision.
hubert.reinterpretcast added a comment.

Patch LGTM. Thanks for looking into the concerns.

================
Comment at: llvm/tools/llc/llc.cpp:513
+      if (!Options.DataSections)
+        reportError("-mroptr option must be used with -data-sections",
+                    InputFilename);
----------------
qiongsiwu1 wrote:
> hubert.reinterpretcast wrote:
> > hubert.reinterpretcast wrote:
> > > See https://reviews.llvm.org/D144190#inline-1406700 for why having this diagnostic in `llc` may not be the best.
> > Since the back end now errors out reliably (`report_fatal_error`), the comment linked above for the other patch implies that we can remove this `llc` diagnostic in this patch. Then we can land this patch first.
> > Since the back end now errors out reliably (`report_fatal_error`), the comment linked above for the other patch implies that we can remove this `llc` diagnostic in this patch. Then we can land this patch first.
> 
> It seems that the `report_fatal_error` mechanism works more like an assert. `llc` crashes when encountering a `report_fatal_error` function. It is probably not ideal that we crash when `-data-sections=false` is passed through the command line. Should we keep this `llc` check? I am inclined to do so because I don't think we should allow `llc` to crash. That said, if the crashing behaviour is acceptable, I will remove the `-data-sections=false` test case, and remove this check. Alternatively, there may be other error reporting mechanisms in the backend that I am not aware of that can generate a valid error message without crashing. If there are, I will adopt them so we can report the error in the backend, and not crash. 
Since we have the diagnostic in `llc` (and for `clang -cc1` in the other patch) already, and it is the most user-friendly behaviour, I think we are good to close on this.

Just noting that the `report_fatal_error` behaviour may be acceptable (in case we end up working on something similar in the future).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144189