This is an archive of the discontinued LLVM Phabricator instance.

prfchwintrin.h: Make _m_prefetchw take a pointer to volatile (PR49124)
ClosedPublic

Authored by hans on Jul 26 2021, 6:15 AM.

Details

Summary

For some reason, Microsoft declares _m_prefetch to take a const void*, but _m_prefetchw to take a /volatile/ const void*.

I can't think of any downside to just casting away the volatile here? (Besides having to suppress the warning in a somewhat ugly way.)

Diff Detail

Event Timeline

hans requested review of this revision.Jul 26 2021, 6:15 AM
hans created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJul 26 2021, 6:15 AM
rnk added a comment.Jul 26 2021, 11:50 AM

Does this match GCC? They also provide this Intel intrinsic.

Seems reasonable in principle, though.

pengfei added inline comments.Jul 26 2021, 8:19 PM
clang/lib/Headers/prfchwintrin.h
54

Can we declare __builtin_prefetch to volatile one in Builtins.def:

BUILTIN(__builtin_prefetch, "vvCD*.", "nc")
hans added a comment.Jul 27 2021, 2:18 AM

Does this match GCC? They also provide this Intel intrinsic.

No, they define it without volatile (and also without const): https://github.com/gcc-mirror/gcc/blob/releases/gcc-11.1.0/gcc/config/i386/prfchwintrin.h#L32

But maybe their header doesn't get included together with Microsoft's header?

clang/lib/Headers/prfchwintrin.h
54

We could, and that would remove the need for the cast, but I'm not sure it would make sense in itself. Since _m_prefetchw is the odd one here, I think it makes sense to do the fix in its implementation.

rnk added a comment.Jul 27 2021, 12:30 PM

Does this match GCC? They also provide this Intel intrinsic.

No, they define it without volatile (and also without const): https://github.com/gcc-mirror/gcc/blob/releases/gcc-11.1.0/gcc/config/i386/prfchwintrin.h#L32

But maybe their header doesn't get included together with Microsoft's header?

Sure, they won't be included together, but will GCC users get warnings from passing non-volatile pointers to _mm_prefetchw? If we change these qualifiers to match MSVC, will GCC/Clang users be able to observe any change in behavior?

hans added a comment.Jul 28 2021, 2:05 AM

Sure, they won't be included together, but will GCC users get warnings from passing non-volatile pointers to _mm_prefetchw? If we change these qualifiers to match MSVC, will GCC/Clang users be able to observe any change in behavior?

I don't think that should cause any problems. Passing a less qualified pointer to a more cv-qualified parameter should be fine, e.g.

void f(volatile void* p);

void g(void *p) {
  f(p);
}
rnk accepted this revision.Jul 28 2021, 12:47 PM

I don't think that should cause any problems. Passing a less qualified pointer to a more cv-qualified parameter should be fine, e.g.

OK, I'm happy then.

clang/lib/Headers/prfchwintrin.h
54

I agree with @hans, I think leaving the prototype of __builtin_prefetch as it is seems reasonable, since it is used elsewhere.

This revision is now accepted and ready to land.Jul 28 2021, 12:47 PM
This revision was landed with ongoing or failed builds.Aug 2 2021, 6:16 AM
This revision was automatically updated to reflect the committed changes.