[PATCH] D156497: [BPF] report fatal error rather than miscompile

Wed Aug 2 11:14:35 PDT 2023

tamird added a comment.

In D156497#4554896 <https://reviews.llvm.org/D156497#4554896>, @eddyz87 wrote:

> I grepped the code base and see that finalizeLowering() logic is customized only in a handful of cases:
>
> - llvm/lib/Target/ARM/ARMISelLowering.cpp
> - llvm/lib/Target/AMDGPU/SIISelLowering.cpp
> - llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
>
> In all these cases there is no similar logic with regards to error handling.
>
> We have the following "fail" messages in the BPFISelLowering:
>
> - "stack arguments are not supported"
> - "variadic functions are not supported"
> - "aggregate returns are not supported"
> - "too many arguments"
> - "pass by value not supported"
> - "A call to built-in function '$func' is not supported." (this one is for ExternalSymbolSDNode)
> - "aggregate returns are not supported"
> - "only small returns supported"
>
> There are no true warnings here, every case is actually an error. (Maybe with exception for ExternalSymbolSDNode which just should be allowed). From my practice I rarely hit even one instance of such errors => it might be the case that capability to report multiple errors at once is not that important. Maybe just update `fail` function to call `report_fatal_error()` internally?
>
> BPFISelLowering is a field in BPFSubtarget, as far as I understand sub-target is a singleton => lowering instance is also a singleton, thus having a global state there is not ideal.
>
> @yonghong-song , what do you think?

>From the description:

> Note also that 8e6aab3 (D21368 <https://reviews.llvm.org/D21368>) removed the exit-on-error flag which is necessary for the unit tests with this change.

I think that even if everyone agreed that crashing is acceptable we'd have to find a solution that would allow the tests to pass. Any ideas?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156497