Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement volatile loads and stores #167

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Implement volatile loads and stores #167

wants to merge 4 commits into from

Conversation

yvt
Copy link
Contributor

@yvt yvt commented Apr 26, 2022

Fixes #166.

antoyo
antoyo reviewed Apr 26, 2022
View reviewed changes
Copy link
Contributor

@antoyo antoyo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Thanks!

src/builder.rs
let loaded_value = function.new_local(None, value_type, &format!("loadedValue{}", unsafe { RETURN_VALUE_COUNT }));
block.add_assignment(None, loaded_value, deref);
loaded_value.to_rvalue()
// TODO: handle non-natural alignments
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rust repo requires that TODO should be annotated with a name. If you don't plan to fix this later, just put my username there.

src/builder.rs
modified_destination_type = modified_destination_type.make_volatile();
}

// FIXME: The type checking in `add_assignment` removes only one
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for FIXME.

src/builder.rs
let modified_ptr = self.cx.context.new_bitcast(None, ptr, modified_destination_type.make_pointer());
let modified_destination = modified_ptr.dereference(None);
self.llbb().add_assignment(None, modified_destination, val);
// TODO: handle `MemFlags::NONTEMPORAL`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

src/builder.rs
let value_type = deref.get_type();

// If `is_volatile == true`, convert `ptr` to `volatile value_type *`
// and try dereference again. (libgccjit doesn't provide a way to get
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if that's what you're talking about, but an alternative is to do:

ptr.get_type().get_pointee()

Copy link
Contributor Author

@yvt yvt Apr 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that's exactly what I was looking for.

@antoyo
Copy link
Contributor

antoyo commented Apr 26, 2022

The CI failed for libgccjit12, btw. Not sure if it's normal.

@yvt
Copy link
Contributor Author

yvt commented Apr 27, 2022

Not normal, I think. Also, I found that volatile reads are still being eliminated by dse pass, which I'm looking into.

p_count.write_volatile(0);
STORAGE.add(0).read_volatile();
STORAGE.add(PAGE_SIZE).read_volatile();
STORAGE.add(0).write_volatile(1);

fake.c.040t.mergephi1:

  # DEBUG INLINE_ENTRY _ZN4core3ptr7mut_ptr31__LT_impl_u20__BP_mut_u20_T_GT_14write_volatile17hb4eb8141481df2c1E
  _ZN8volatile5COUNT17h55d9a409972f8391E = 0;
  # DEBUG INLINE_ENTRY _ZN4core3ptr7mut_ptr31__LT_impl_u20__BP_mut_u20_T_GT_3add17h994bc78b46d6ca3bE
  # DEBUG INLINE_ENTRY _ZN4core3ptr7mut_ptr31__LT_impl_u20__BP_mut_u20_T_GT_13read_volatile17hb2bc363effab5c7cE
  loadedValue3_87 = MEM[(volatile unsigned char *)ptrReturnValue11_69];
  # DEBUG INLINE_ENTRY _ZN4core3ptr7mut_ptr31__LT_impl_u20__BP_mut_u20_T_GT_3add17h994bc78b46d6ca3bE
  # DEBUG INLINE_ENTRY _ZN4core3ptr7mut_ptr31__LT_impl_u20__BP_mut_u20_T_GT_13read_volatile17hb2bc363effab5c7cE
  loadedValue3_97 = MEM[(volatile unsigned char *)ptrReturnValue11_69 + 32768B];
  # DEBUG INLINE_ENTRY _ZN4core3ptr7mut_ptr31__LT_impl_u20__BP_mut_u20_T_GT_3add17h994bc78b46d6ca3bE
  # DEBUG INLINE_ENTRY _ZN4core3ptr7mut_ptr31__LT_impl_u20__BP_mut_u20_T_GT_14write_volatile17h2a645162d347c6a8E
  MEM[(volatile unsigned char *)ptrReturnValue11_69] = 1;

fake.c.041t.dse1:

  # DEBUG INLINE_ENTRY _ZN4core3ptr7mut_ptr31__LT_impl_u20__BP_mut_u20_T_GT_14write_volatile17hb4eb8141481df2c1E
  _ZN8volatile5COUNT17h55d9a409972f8391E = 0;
  # DEBUG INLINE_ENTRY _ZN4core3ptr7mut_ptr31__LT_impl_u20__BP_mut_u20_T_GT_3add17h994bc78b46d6ca3bE
  # DEBUG INLINE_ENTRY _ZN4core3ptr7mut_ptr31__LT_impl_u20__BP_mut_u20_T_GT_13read_volatile17hb2bc363effab5c7cE
  # DEBUG INLINE_ENTRY _ZN4core3ptr7mut_ptr31__LT_impl_u20__BP_mut_u20_T_GT_3add17h994bc78b46d6ca3bE
  # DEBUG INLINE_ENTRY _ZN4core3ptr7mut_ptr31__LT_impl_u20__BP_mut_u20_T_GT_13read_volatile17hb2bc363effab5c7cE
  # DEBUG INLINE_ENTRY _ZN4core3ptr7mut_ptr31__LT_impl_u20__BP_mut_u20_T_GT_3add17h994bc78b46d6ca3bE
  # DEBUG INLINE_ENTRY _ZN4core3ptr7mut_ptr31__LT_impl_u20__BP_mut_u20_T_GT_14write_volatile17h2a645162d347c6a8E
  MEM[(volatile unsigned char *)ptrReturnValue11_69] = 1;

@YakoYakoYokuYoku YakoYakoYokuYoku mentioned this pull request Jan 4, 2024
Open
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antoyo antoyo antoyo left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Volatile loads and stores aren't honored
2 participants
@yvt @antoyo