This is an archive of the discontinued LLVM Phabricator instance.

[Verifier][NFC] Refactor check for associated metadata to allow multiple operands on AIX
Changes PlannedPublic

Authored by tingwang on Mar 9 2023, 10:03 PM.

Details

Reviewers
arsenm
shchenz
hubert.reinterpretcast
nemanjai
cebowleratibm
MaskRay
Group Reviewers
Restricted Project
Summary

Currently associated metadata is limited to have single operand.

Patch https://reviews.llvm.org/D125095 intends to use associated metadata to express the associations between global objects on AIX. The association could be one to multiple, for example one static variable could have associated with init and term functions.

This patch intends to setup context to check multiple operands for TT.isOSAIX().

Diff Detail

Event Timeline

tingwang created this revision.Mar 9 2023, 10:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 9 2023, 10:03 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
tingwang requested review of this revision.Mar 9 2023, 10:03 PM
arsenm requested changes to this revision.Mar 10 2023, 3:37 AM

Missing tests and associated langref changes

This revision now requires changes to proceed.Mar 10 2023, 3:37 AM
tingwang updated this revision to Diff 504952.Mar 13 2023, 11:34 PM
tingwang added a reviewer: Restricted Project.

Address comments:
(1) Add test case to show how associated metadata will be used on AIX (currently not supported, marked with XFAIL).
(2) Add langref changes to explain.

Herald added a project: Restricted Project. · View Herald TranscriptMar 13 2023, 11:34 PM

Missing verifier test in llvm/test/Verifier (e.g., the ones I added in 87f2e9448e82bbed4ac59bb61bea03256aa5f4de)

clang/test/CodeGen/PowerPC/aix-init-ref-null.cpp
2 ↗(On Diff #504952)

A test run with not and a specific error message check is more reliable

tingwang updated this revision to Diff 505372.Mar 14 2023, 11:08 PM

Address comments:
(1) Duplicated associated test cases to make copies for AIX XCOFF, and added multiple operands and null operand as legal cases.
(2) Updated verifier logic to check for AIX target.
(3) Moved XFAIL check to specific lines.

tingwang added inline comments.Mar 14 2023, 11:12 PM
clang/test/CodeGen/PowerPC/aix-init-ref-null.cpp
2 ↗(On Diff #504952)

Thank you Matt! Currently there is no error message for this case, so I moved the XFAIL to CHECK lines. Hope that is fine.

tingwang updated this revision to Diff 505709.Mar 16 2023, 12:03 AM

Add empty case in Verifier/associated-metadata-aix-xcoff.ll

arsenm added inline comments.Jun 9 2023, 6:38 PM
clang/test/CodeGen/PowerPC/aix-init-ref-null.cpp
1 ↗(On Diff #505709)

Don't use -O3, maybe -O1 -disable-llvm-passes

1 ↗(On Diff #505709)

Use generated checks with --check-globals?

22 ↗(On Diff #505709)

XFAIL-CHECK doesn't do anything

clang/test/CodeGen/PowerPC/aix-ref-static-var.cpp
11 ↗(On Diff #505709)

XFAIL-CHECK doesn't do anything

tingwang planned changes to this revision.EditedJun 12 2023, 1:04 AM

Hi Matt @arsenm, thank you for your comments, and I will incorporate your suggestions in the next version.

Since the review process of dependent patch https://reviews.llvm.org/D125095 didn't get any progress for a while..., I'm worried that discussing the verification logic maybe a little bit early. I plan to suspend this patch until I got any progress on D125095. Thank you!