Missing extension attributes in tests

[JonPsson1]

As argument extensions are needed for correctness, I have been working on going through the code base for cases where they are missing. In mlir, I have now found a few, that I would need help with. In particular, I don't know where these functions are emitted, or how to add the proper Attributes (CreateRuntimeFunction? /.td file?). These are the tests that fail, with the problematic function printed as well:

FAIL: MLIR-Unit :: ExecutionEngine/./MLIRExecutionEngineTests/6/12 (11 of 2540)
******************** TEST 'MLIR-Unit :: ExecutionEngine/./MLIRExecutionEngineTests/6/12' FAILED ********************
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from MLIRExecutionEngine
[ RUN      ] MLIRExecutionEngine.AddInteger
ERROR: Missing extension attribute of returned value from function:  // and arg
i32 @foo(i32)

FAIL: MLIR :: CAPI/execution_engine.c (12 of 2540)
******************** TEST 'MLIR :: CAPI/execution_engine.c' FAILED ********************
Running test 'testSimpleExecution'
ERROR: Missing extension attribute of returned value from function:  // and arg
range(i32 0, -1) i32 @add(i32)

FAIL: MLIR :: mlir-cpu-runner/simple.mlir (13 of 2540)
******************** TEST 'MLIR :: mlir-cpu-runner/simple.mlir' FAILED ********************
# | ERROR: Missing extension attribute of returned value from function:
# | i32 @int32_main()

FAIL: MLIR-Unit :: ExecutionEngine/./MLIRExecutionEngineTests/11/12 (15 of 2540)
******************** TEST 'MLIR-Unit :: ExecutionEngine/./MLIRExecutionEngineTests/11/12' FAILED ********************
ERROR: Missing extension attribute of passed value in call to function:
Callee:  void @_mlir_ciface_callback(ptr, i32)
Caller:  void @callback(ptr, ptr, i64, i64, i64, i64, i64, i32)

Thanks for any help here. Adding people I find in the logs:
@shahidact @dcaballe @banach-space @ienkovich @tblah @paulwalker-arm @brod4910 @CoTinker @Groverkss

[llvmbot]

@llvm/issue-subscribers-mlir

Author: Jonas Paulsson (JonPsson1)

As argument extensions are needed for correctness, I have been working on going through the code base for cases where they are missing. In mlir, I have now found a few, that I would need help with. In particular, I don't know where these functions are emitted, or how to add the proper Attributes (CreateRuntimeFunction? /.td file?). These are the tests that fail, with the problematic function printed as well:

FAIL: MLIR-Unit :: ExecutionEngine/./MLIRExecutionEngineTests/6/12 (11 of 2540)
******************** TEST 'MLIR-Unit :: ExecutionEngine/./MLIRExecutionEngineTests/6/12' FAILED ********************
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from MLIRExecutionEngine
[ RUN      ] MLIRExecutionEngine.AddInteger
ERROR: Missing extension attribute of returned value from function:  // and arg
i32 @<!-- -->foo(i32)

FAIL: MLIR :: CAPI/execution_engine.c (12 of 2540)
******************** TEST 'MLIR :: CAPI/execution_engine.c' FAILED ********************
Running test 'testSimpleExecution'
ERROR: Missing extension attribute of returned value from function:  // and arg
range(i32 0, -1) i32 @<!-- -->add(i32)

FAIL: MLIR :: mlir-cpu-runner/simple.mlir (13 of 2540)
******************** TEST 'MLIR :: mlir-cpu-runner/simple.mlir' FAILED ********************
# | ERROR: Missing extension attribute of returned value from function:
# | i32 @<!-- -->int32_main()

FAIL: MLIR-Unit :: ExecutionEngine/./MLIRExecutionEngineTests/11/12 (15 of 2540)
******************** TEST 'MLIR-Unit :: ExecutionEngine/./MLIRExecutionEngineTests/11/12' FAILED ********************
ERROR: Missing extension attribute of passed value in call to function:
Callee:  void @<!-- -->_mlir_ciface_callback(ptr, i32)
Caller:  void @<!-- -->callback(ptr, ptr, i64, i64, i64, i64, i64, i32)

Thanks for any help here. Adding people I find in the logs:
@shahidact @dcaballe @banach-space @ienkovich @tblah @paulwalker-arm @brod4910 @CoTinker @Groverkss

[banach-space]

Thanks for raising this! What are "argument extensions"? 😅

[JonPsson1]

Thanks for raising this! What are "argument extensions"? 😅

A few targets - like SystemZ - has per their ABI a requirement that integer arguments are extended before passed / returned to another function. On SystemZ, e.g. an i32 argument must be either sign or zero extended to the full register width (64 bits). This really must be done by the caller (/return instruction), as the other function is free to assume that this is always done.

The general reasoning for these extension attributes are that some targets care about them, and targets who don't are free to ignore them if present.

As an example, I recently fixed a case of this: 76a52db. As the test shows, the attributes are added on SystemZ.

[JonPsson1]

Please note that this verification step producing these errors only exists so far in the SystemZ backend, so you will not see these errors normally.

[JonPsson1]

So for example, I can see that mlir/test/mlir-cpu-runner/simple.mlir fails because it generates the LLVM I/R

i32 @int32_main()

which should be

signext i32 @int32_main()

(assuming that @int32_main return value is signed)

I wonder where exactly this:

llvm.func @int32_main() -> i32

becomes the LLVM I/R function above..?

[JonPsson1]

I have managed to track down the actual creation of the IR Function to happen in ModuleTranslation::convertFunctionSignatures(). What I see is that in this .mlir test file, there exists:

llvm.func @int32_main() -> i32 {...

So this is an arbitrarily defined function without any signext/zeroext attribute on the return value. Now I wonder if there is even such a thing in the MLIR module?

• Can all these mlir functions be considered "internal" so that it would be safe to omit these attributres? Or is there some cases where such a function could be passed to an external function?
• I suppose there can be calls to external functions made? It would make sense to add the correct attributes in such cases but then again I don't know if the MLIR module supports these attributes?

If this is a huge task, or if it's not relevant, perhaps the verification step could be disabled (for now) like, for llc (Target->Options.VerifyArgABICompliance = 0;)

@nikic Any thoughts on this?

[zero9178]

I have managed to track down the actual creation of the IR Function to happen in ModuleTranslation::convertFunctionSignatures(). What I see is that in this .mlir test file, there exists:

llvm.func @int32_main() -> i32 {...

So this is an arbitrarily defined function without any signext/zeroext attribute on the return value. Now I wonder if there is even such a thing in the MLIR module?

MLIR's LLVM dialect also supports signext and zeroext yes! See for example: https://godbolt.org/z/b7bs1fGT9

That said, the tests you are looking at use LLVM IR as input, right? I think the issue you're mainly hitting is that LLVM IR tends to very target dependent already, especially in the regards of ABI. Sure, you could add ABI attributes and make it work for SystemZ but then it might not work on other targets.
In the specific case of zeroext and signext I believe it might be safe from an implementation detail perspective, but the language reference likely disagrees.

IMO it'd be reasonable to disable these tests on SystemZ using a // requires comment. You could separately duplicate the test and specialize it with SystemZ and make the test run on SystemZ only.

[nikic]

I think for these tests it would make sense to use VerifyArgABICompliance = 0. It's my understanding that they aren't intended to use any specific ABI, so we shouldn't validate it.

(Now, if we say started with ClangIR and then ended up in this position, that would be a different matter.)

[JonPsson1]

MLIR's LLVM dialect also supports signext and zeroext yes! See for example: https://godbolt.org/z/b7bs1fGT9

thanks! - good to know.

That said, the tests you are looking at use LLVM IR as input, right?

In at least one of them, the input is an mlir module (see previous post). May be LLVM IR in the others...

I think the issue you're mainly hitting is that LLVM IR tends to very target dependent already, especially in the regards of ABI. Sure, you could add ABI attributes and make it work for SystemZ but then it might not work on other targets. In the specific case of zeroext and signext I believe it might be safe from an implementation detail perspective, but the language reference likely disagrees.

IMO it'd be reasonable to disable these tests on SystemZ using a // requires comment. You could separately duplicate the test and specialize it with SystemZ and make the test run on SystemZ only.

That seems like a reasonable option, at least for tests that were written in LLVM I/R. On the other hand, like @nikic wrote, these tests aren't intented for a specific ABI, so if it would be possible to use the VerifyArgABICompliance TargetInfo flag that might be simple enough (I suppose for mlir-cpu-runner.cpp this might work?)

[JonPsson1]

I first tried to see if I could use TargetOptions like with llc, but that did not seem straightforward as it is not declared anywhere as in llc.cpp, at least not what I could see.

I then tried to update the tests as best I could, which seems to work fine. Instead of duplicating the test, I disable the check on the RUN-line, which I guess works well as it is just in one file, I'd say.

Patch proposed here: #116314