This is an archive of the discontinued LLVM Phabricator instance.

[builtins] Add __builtin_assume_separate_storage.

Authored by davidtgoldblatt on Oct 21 2022, 4:37 PM.



Just plumbing from the language level to the assume intrinsics with
separate_storage operand bundles.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptOct 21 2022, 4:37 PM
Matt added a subscriber: Matt.Oct 25 2022, 11:20 AM

Updating per review comments.

davidtgoldblatt retitled this revision from [builtins] add separate storage builtin support. to [builtins] Add __builtin_assume_separate_storage..
davidtgoldblatt edited the summary of this revision. (Show Details)

Updating per review comments.

davidtgoldblatt published this revision for review.Nov 17 2022, 4:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 17 2022, 4:53 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Fix out-of-date comment.

Per review comments.

bruno requested changes to this revision.Dec 20 2022, 6:34 AM

Nice, thanks for adding the builtin layer.

7808 ↗(On Diff #481770)

Can you prefix this with a FIXME/TODO?

7810 ↗(On Diff #481770)

IIUC, seems like there a two pieces here you can already cover as part of this patch:

  • Check for exact two arguments. If in the future an optional multi-arg variadic version is to be supported the checking could be enhanced/changed to support that.
  • Check both are pointers. I noticed that in the testcase you rely on incompatible integer to pointer conversion and friends to catch some of these, but a more user friendly diagnostic could be explicit about the expectations. In case these current error diagnostics you are relying run before this sema check, perhaps a note diagnostic could help clarify.
This revision now requires changes to proceed.Dec 20 2022, 6:34 AM
bruno added a comment.Dec 20 2022, 6:44 AM

Additionally, this likely deserves an entry in ./clang/docs/ReleaseNotes.rst.

Summarizing a brief offline discussion: I think it's probably fine to just get rid of the SemaChecking changes entirely; type-checking here is done by the generalized builtin-handling code. I'll do the change if there's no objection.

In addition to the release notes addition, I also added an entry in LanguageExtensions.rst.

Use utils/ for the codegen test.

bruno added inline comments.Mar 15 2023, 11:23 AM

Not necessarily a blocker, but it seems like some of these things you mention can actually be caught by diagnostics without too much effort? Any plans to add them?


I hadn't planned to (at least in the short term). Practically I expect uses of this to be mostly backed out from looking at bad assembly (this is the way I've been using it so far in experimentation). You wouldn't generally expect people to want to try to express "these two struct fields don't alias" and so on because alias analysis can already handle those cases fairly well.

My inclination would be to wait on the diagnostics until we see if it's a real problem, but I'm not strongly opposed if you'd really like them in v1. (Although in that case I'll probably bug you for some help with where / how to put the diagnostics).

bruno accepted this revision.Mar 21 2023, 11:19 AM



My inclination would be to wait on the diagnostics until we see if it's a real problem

Sounds fair! Feel free to bug me when/if you decide to explore that route.

This revision is now accepted and ready to land.Mar 21 2023, 11:19 AM
This revision was automatically updated to reflect the committed changes.
OfekShilon added inline comments.

+2c: as of today tbaa *doesn't* handle well aliasing of two struct fields. There was some work to mitigate it by @kosarev ( but it is still hidden behind a -new-struct-path-tbaa switch. Example: