This is an archive of the discontinued LLVM Phabricator instance.

[mlir][SystemZ] Disable `-fno-semantic-interposition` option.
ClosedPublic

Authored by imaihal on Jul 5 2021, 7:19 PM.

Details

Summary

-fno-semantic-interposition was added for GCC in D102453, but MLIR test
on SystemZ failed when GCC version is less than 10.3 because of GCC bug.
This patch disables the option only on SystemZ when using GCC version less than 10.3.

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Diff Detail

Event Timeline

imaihal created this revision.Jul 5 2021, 7:19 PM
imaihal requested review of this revision.Jul 5 2021, 7:19 PM
imaihal added a subscriber: MaskRay.Jul 6 2021, 1:08 AM

After D102453, MLIR installation test (cmake --build . --target check-mlir) issues 32 errors on SystemZ with GCC. This errors can be avoided by disabling fno-semantic-interposition. I used g++ (Ubuntu 9.4.0-1ubuntu1~18.04) 9.4.0.

One of the errors is as follows. Also in other errors, operands seems to be broken.

llvm-project/mlir/test/Conversion/GPUCommon/lower-memcpy-to-gpu-runtime-calls.mlir:13:11: error: 'llvm.getelementptr' op expected 1 or more operands
    %t1 = gpu.memcpy async [%t0] %dst, %src : memref<7xf32, 1>, memref<7xf32>

Following is generated inc file (LLVMOps.cpp.inc). I think the operands is broken, but when I inserted printf here, the error disappears.

void GEPOp::build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, Type resultType, ValueRange operands, ArrayRef<NamedAttribute> attributes ) {
    if (resultType) odsState.addTypes(resultType);
    odsState.addOperands(operands);
    for (auto namedAttr : attributes) {
      odsState.addAttribute(namedAttr.first, namedAttr.second);
    }
  
}

Hi, @MaskRay This patch modifies your patch (D102453). Do you have any suggestions or comments?

MaskRay added a comment.EditedJul 6 2021, 10:24 AM

After D102453, MLIR installation test (cmake --build . --target check-mlir) issues 32 errors on SystemZ with GCC. This errors can be avoided by disabling fno-semantic-interposition. I used g++ (Ubuntu 9.4.0-1ubuntu1~18.04) 9.4.0.

One of the errors is as follows. Also in other errors, operands seems to be broken.

llvm-project/mlir/test/Conversion/GPUCommon/lower-memcpy-to-gpu-runtime-calls.mlir:13:11: error: 'llvm.getelementptr' op expected 1 or more operands
    %t1 = gpu.memcpy async [%t0] %dst, %src : memref<7xf32, 1>, memref<7xf32>

Following is generated inc file (LLVMOps.cpp.inc). I think the operands is broken, but when I inserted printf here, the error disappears.

void GEPOp::build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, Type resultType, ValueRange operands, ArrayRef<NamedAttribute> attributes ) {
    if (resultType) odsState.addTypes(resultType);
    odsState.addOperands(operands);
    for (auto namedAttr : attributes) {
      odsState.addAttribute(namedAttr.first, namedAttr.second);
    }
  
}

Hi, @MaskRay This patch modifies your patch (D102453). Do you have any suggestions or comments?

I don't mind. I suspect there is a gcc SystemZ bug but I don't have access to a SystemZ machine.
Worth filing a gcc bug if you can spend more time on investigating the issue.

imaihal updated this revision to Diff 356888.EditedJul 7 2021, 1:07 AM

Condition to disable the option was updated since we found GCC version >= 11 is no problem.

imaihal retitled this revision from [mlir][System_Z] Disable `-fno-semantic-interposition` option. to [mlir][SystemZ] Disable `-fno-semantic-interposition` option..Jul 7 2021, 1:08 AM
imaihal added a subscriber: phosek.Jul 7 2021, 1:17 AM

I don't mind. I suspect there is a gcc SystemZ bug but I don't have access to a SystemZ machine.
Worth filing a gcc bug if you can spend more time on investigating the issue.

@MaskRay Thanks for your comments. We have not found the actual bug yet, but it seems it was solved in GCC version >= 11. So I updated the condition in this patch.

@MaskRay @phosek Is it possible to be a reviewer of this patch? I asked because you were a developer and a reviewer of D102453.

imaihal updated this revision to Diff 356953.Jul 7 2021, 7:49 AM

Updated the condition to disable the option. It is disabled when GCC version is less than 10.3.

imaihal edited the summary of this revision. (Show Details)Jul 7 2021, 8:18 AM
MaskRay added a comment.EditedJul 7 2021, 4:19 PM

It'd be good to know which gcc bug this is.

Perhaps just exclude SystemZ for all configurations, instead of checking "mlir" IN_LIST LLVM_ENABLE_PROJECTS. Include a link to the bug report.

We generally still want -fno-semantic-interposition because gcc -fno-semantic-interposition built clang can be 3% faster on some benchmarks.

imaihal updated this revision to Diff 357405.Jul 8 2021, 7:53 PM

Removed checking for MLIR build. Now the option is disabled for all configuration of SystemZ.

imaihal edited the summary of this revision. (Show Details)Jul 8 2021, 7:54 PM

@MaskRay Thanks for comments.

Perhaps just exclude SystemZ for all configurations, instead of checking "mlir" IN_LIST LLVM_ENABLE_PROJECTS.

I removed the check.

Include a link to the bug report.

We are investigating the bug report.

Thanks.

MaskRay accepted this revision.Jul 8 2021, 8:09 PM

Looks great!

llvm/cmake/modules/HandleLLVMOptions.cmake
321

GCC<11 has bugs on SystemZ, so don't use the option for older GCC.

This revision is now accepted and ready to land.Jul 8 2021, 8:09 PM

Oh, new GCC works? That's great.

imaihal updated this revision to Diff 357410.Jul 8 2021, 8:39 PM

Updated the comment. We confirmed GCC version >= 10.3 works, not >= 11.

imaihal updated this revision to Diff 357411.Jul 8 2021, 8:49 PM
imaihal marked an inline comment as done.

Updated the comment again to reflect @MaskRay`s comment.

imaihal added a comment.EditedJul 8 2021, 9:33 PM

@MaskRay Yes. New GCC (>= 10.3) works.

Don't I need to include the bug report? It seems it takes much time to identify which bug fix(bug report) solved this issue..
If so, could you land this patch? I don't have commit access.

@MaskRay Yes. New GCC (>= 10.3) works.

Don't I need to include the bug report? It seems it takes much time to identify which bug fix(bug report) solved this issue..
If so, could you land this patch? I don't have commit access.

Not required. I initially thought this affects latest GCC, then having a bug number will help us to track the progress and ensure the workaround can be removed in time.

Since this is actually a problem fixed in latest versions, a bug reference is less important. But if you can identify the issue it'd be good to add a note as well, but not strictly required.

I'll commit it on your behalf