[PATCH] D156497: [BPF] Emit UNDEF rather than constant 0

[Yonghong Song via Phabricator via llvm-commits]

yonghong-song added a comment.

> We must be misunderstanding each other. This patch doesn't change the error messages presented to the user at compilation time.
>
> The purpose of the tests in this patch are to show how it is trivially easy to incorrectly use the LLVM C API such that diagnostics "errors" do not halt compilation.
>
> The purpose of the non-test changes in this patch are to cause the resulting program to fail to verify. Before this patch the resulting program would appear to be valid, but violate the user's intent at runtime.

Okay, I did a little more study by examining other backends by searching key word 'diagnose'.

For example, I found two cases for X86:

  X86/X86InsertPrefetch.cpp:    Ctx.diagnose(DiagnosticInfoSampleProfile(Filename, Msg,
  X86/X86ISelLoweringCall.cpp:  DAG.getContext()->diagnose(

For the second case (X86ISelLoweringCall.cpp), they added after diagnose

  VA.convertToReg(X86::FP0); // Set reg to FP0, avoid hitting asserts.

to avoid hitting asserts. This is similar to BPF one to add InVals.push_back() to avoid asserts later.
For X86InsertPrefetch.cpp, they simply just return to the caller with error.

For RISCV,

  RISCV/RISCVISelLowering.cpp:    MF.getFunction().getContext().diagnose(DiagnosticInfoUnsupported{
  RISCV/RISCVISelLowering.cpp:        MF.getFunction().getContext().diagnose(DiagnosticInfoUnsupported{
  RISCV/RISCVISelLowering.cpp:        MF.getFunction().getContext().diagnose(DiagnosticInfoUnsupported{
  RISCV/RISCVISelLowering.cpp:    F.getContext().diagnose(DiagnosticInfoUnsupported{

it seems after diagnose() in error branch, they do not have any special processing and assuming everything is normal after diagnose.

AMGPU is another example

  AMDGPU/SIISelLowering.cpp:    DAG.getContext()->diagnose(NoGraphicsHSA);
  AMDGPU/SIISelLowering.cpp:    Ctx.diagnose(NoTrap);
  AMDGPU/SIISelLowering.cpp:  DAG.getContext()->diagnose(InvalidAddrSpaceCast);
  AMDGPU/SIISelLowering.cpp:  DAG.getContext()->diagnose(BadIntrin);
  AMDGPU/SIISelLowering.cpp:  DAG.getContext()->diagnose(BadIntrin);
  AMDGPU/SIISelLowering.cpp:      DAG.getContext()->diagnose(BadIntrin);
  AMDGPU/SIISelLowering.cpp:      DAG.getContext()->diagnose(BadIntrin);
  AMDGPU/SIInstrInfo.cpp:  C.diagnose(IllegalCopy);

It looks like for AMDGPU, it does use UNDEF:

  ...
  DAG.getContext()->diagnose(NoGraphicsHSA);
  return DAG.getEntryNode();
  ...
  Ctx.diagnose(NoTrap);
  return Chain;
  ...
  DAG.getContext()->diagnose(InvalidAddrSpaceCast);
  return DAG.getUNDEF(ASC->getValueType(0));
  ...
  DAG.getContext()->diagnose(BadIntrin);  <=== 3 times
  return DAG.getUNDEF(VT);
  ...
      if (!Subtarget->hasCompressedExport()) {
      DiagnosticInfoUnsupported BadIntrin(
          DAG.getMachineFunction().getFunction(),
          "intrinsic not supported on subtarget", DL.getDebugLoc());
      DAG.getContext()->diagnose(BadIntrin);   <=== no special handling after this
    } 
    SDValue Src0 = Op.getOperand(4);

So looks like the approach largely in three categories:

- add change to avoid asserts later (bpf is doing InVals.push_back() due to the same reason)
- do not need to anything speical (bpf has to add InVals.push_back() to avoid later assertion)
- using UNDEF to proceed.

So I think using UNDEF is okay here for BPF. This does not affect clang but it may improve for other projects (rust or others) which use llvm APIs for bpf backend.

So please make the following changes to the diff:

- remove bpf-unspported-emit-c. I think this is not needed. But please explain in the commit message how it is possible to have a custom diagnose handler which can avoid 'exit()' and proceed with compilation.
- please describe why UNDEF is used instead of 'constant 0' based on analysis with the usage of other backends.
- please describe with your custom diagnose handler, what kind of error message (or lack of error message) with and without this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156497