This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC][AIX] Add warning when alignment is incompatible with XL
ClosedPublic

Authored by ZarkoCA on Jul 8 2021, 2:30 PM.

Details

Summary

https://reviews.llvm.org/D105659 implements ByVal handling in llc but
some cases are not compatible with existing XL compiler on AIX. Adding
a clang warning for such cases.

Diff Detail

Event Timeline

ZarkoCA created this revision.Jul 8 2021, 2:30 PM
ZarkoCA requested review of this revision.Jul 8 2021, 2:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2021, 2:30 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
aaron.ballman added inline comments.Jul 9 2021, 6:29 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
3258–3259

Should we be talking about the AIX XL compiler in a Clang diagnostic?

clang/lib/Sema/SemaDeclAttr.cpp
3963
ZarkoCA updated this revision to Diff 358087.Jul 12 2021, 3:45 PM
  • Warning should only apply to members of struct
  • Re-word warning slightly
ZarkoCA marked an inline comment as done.Jul 12 2021, 3:53 PM
ZarkoCA added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
3258–3259

I see your point. Sorry if this isn't what is supposed to be done or if it doesn't a good precedent.

The reasons for adding this warning is that our back end implementation isn't totally compatible with XL now and, while buggy, users on AIX may expect clang and xlclang to be compatible since AIX is the reference compiler. The xlclang name implies it's clang based and it's possible for users to expect some sort of binary compatibility.

ZarkoCA planned changes to this revision.Jul 13 2021, 6:28 AM
ZarkoCA updated this revision to Diff 358261.Jul 13 2021, 7:16 AM
  • Add a warning group for this warning
aaron.ballman added inline comments.Jul 14 2021, 11:31 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
3258–3259

I see your point. Sorry if this isn't what is supposed to be done or if it doesn't a good precedent.

No worries, it's a good discussion to have! We have some MSVC and GCC compatibility warnings, so there's precedent for naming other compilers. Now that you've moved the diagnostic into an AIX compatibility diagnostic group, I am more comfortable with it. Thanks!

clang/lib/Sema/SemaDeclAttr.cpp
3959–3962
clang/test/Sema/aix-attr-align.c
4

Can you add two more RUN lines where we expect no warnings? One would be from a non AIX triple and the other would be with -Wno-aix-compat specified?

Btw, in case you don't know about it, you can do -verify=off for those RUN lines and add // off-no-diagnostics to the top of the file and that should cover it.

ZarkoCA updated this revision to Diff 358760.Jul 14 2021, 2:46 PM
  • Add more RUN lines to test
  • change if condition to be one line
ZarkoCA marked an inline comment as done.Jul 14 2021, 2:50 PM
ZarkoCA added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
3258–3259

Thanks, glad it's better now.

clang/test/Sema/aix-attr-align.c
4

Thanks for the suggestion to add more lines, that coverage was missing before from the patch.

aaron.ballman added inline comments.Jul 15 2021, 3:54 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
3258–3259

I missed this last time, sorry, but is "arguments" actually necessary for the diagnostic or can that be dropped?

clang/test/Sema/aix-attr-align.c
18–21

Minor formatting nit.

ZarkoCA updated this revision to Diff 358934.Jul 15 2021, 5:33 AM
  • Fix formatting in test case
  • Reword warning message
ZarkoCA marked 2 inline comments as done.Jul 15 2021, 5:34 AM
ZarkoCA added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
3258–3259

It actually isn't correct, the warning should apply only to members of structs. Thanks for bringing it to my attention again, I also missed this the last time.

aaron.ballman accepted this revision.Jul 15 2021, 6:56 AM

LGTM, thank you!

clang/include/clang/Basic/DiagnosticSemaKinds.td
3258–3259

Huttah for code review working as intended! :-)

This revision is now accepted and ready to land.Jul 15 2021, 6:56 AM
ZarkoCA added inline comments.Jul 16 2021, 4:51 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
3258–3259

Indeed, thank you for the timely reviews.

This revision was landed with ongoing or failed builds.Jul 16 2021, 4:52 AM
This revision was automatically updated to reflect the committed changes.