This is an archive of the discontinued LLVM Phabricator instance.

Avoid exporting 80-bit fp functions for architectures other than Intel.
ClosedPublic

Authored by malharJ on Oct 12 2022, 10:37 AM.

Details

Summary

This patch is a partial fix for issue, due to functions affected by D117473.

Implementation details:
The patch essentially creates a new macro if the architecture is either
intel32 or intel64, since the generate-def.pl cannot process boolean algebra
on macros.

Diff Detail

Event Timeline

malharJ created this revision.Oct 12 2022, 10:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 12 2022, 10:37 AM
malharJ requested review of this revision.Oct 12 2022, 10:37 AM
Herald added a project: Restricted Project. · View Herald Transcript
malharJ edited the summary of this revision. (Show Details)Oct 12 2022, 10:39 AM

Just a few comments.

openmp/runtime/src/dllexports
759–762

Should these be guarded?

819–820

Replace these two lines with #ifdef IS_IA_ARCH (and corresponding %endif)

1246–1248

Why aren't the _cpt versions guarded as well?

mgabka added a subscriber: mgabka.Oct 13 2022, 10:36 PM
malharJ updated this revision to Diff 467720.Oct 14 2022, 2:45 AM
malharJ marked an inline comment as done.
  • Added IS_IA_ARCH guard around __kmpc_atomic_float10_*_fp functions
malharJ added inline comments.Oct 14 2022, 2:54 AM
openmp/runtime/src/dllexports
819–820

I'm a little unsure if that is having the same semantics ?
If I understand correctly you have suggested this because of the comment on line 818 ?

but I see

%ifdef arch_32

on line 839 (and it is inside the ifdef)

1246–1248

I've not touched these functions as this patch is mainly a fix (for the github issue: #56349) for
functions affected by one of my earlier patches, D117473.

I can try and make a separate patch later.

malharJ edited the summary of this revision. (Show Details)Oct 14 2022, 2:55 AM
jlpeyton added inline comments.Oct 17 2022, 9:12 AM
openmp/runtime/src/dllexports
819–820

I think this change is OK, or I fail to see any issue.

Just for reference,

  • arch_32 = x86
  • arch_32e = x64
  • arch_64 = unused. It used to mean Itanium, but it is not currently possible to produce it anymore.

So having

# Both x86 and x64
%ifdef IS_IA_ARCH
... Both x86 and x64 exports
%ifdef arch_32
# Only x86 exports
%endif
...
%endif

Seems fine.

malharJ updated this revision to Diff 468360.Oct 17 2022, 4:19 PM
  • replaced %ifndef arch_64, arch_aarch64 with IS_IA_ARCH macro
malharJ marked an inline comment as done.Oct 17 2022, 4:19 PM
malharJ edited the summary of this revision. (Show Details)Oct 19 2022, 3:34 AM
This revision is now accepted and ready to land.Oct 19 2022, 7:31 AM