Page MenuHomePhabricator

Bryce-MW (Bryce Wilson)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 6 2022, 5:58 PM (20 w, 23 h)

Recent Activity

Jan 26 2022

Bryce-MW added inline comments to D117600: [CGCall] Annotate operator new with inaccessiblememonly if AssumeSaneOperatorNew is on.
Jan 26 2022, 10:58 AM · Restricted Project
Bryce-MW updated the diff for D117600: [CGCall] Annotate operator new with inaccessiblememonly if AssumeSaneOperatorNew is on.
  • Use proper attribute
Jan 26 2022, 10:58 AM · Restricted Project

Jan 18 2022

Bryce-MW added a reviewer for D117600: [CGCall] Annotate operator new with inaccessiblememonly if AssumeSaneOperatorNew is on: reames.
Jan 18 2022, 3:04 PM · Restricted Project
Bryce-MW published D117600: [CGCall] Annotate operator new with inaccessiblememonly if AssumeSaneOperatorNew is on for review.
Jan 18 2022, 3:04 PM · Restricted Project
Bryce-MW added a comment to D117180: [BasicAliasAnalysis] Switch from isMallocOrCallocLikeFn to onlyAccessesInaccessibleMemory.

I don't think op new is annotated as inaccessible memory anywhere. I'm working on a patch in D117600. I was just about to submit it for review

Jan 18 2022, 3:01 PM · Restricted Project
Bryce-MW accepted D117571: [LangRef] Clarify that inaccessiblememonly functions are allowed noalias returns.

It was ambiguous before because this is exactly what I thought it meant. Even though the memory becomes accessible after returning, it is not accessible during the call so it should be allowed to be modified. I think this clarification makes it a lot more clear.

Jan 18 2022, 12:34 PM · Restricted Project

Jan 17 2022

Bryce-MW added a comment to D117180: [BasicAliasAnalysis] Switch from isMallocOrCallocLikeFn to onlyAccessesInaccessibleMemory.

That sounds reasonable to me!

Jan 17 2022, 3:47 PM · Restricted Project
Bryce-MW added a comment to D117180: [BasicAliasAnalysis] Switch from isMallocOrCallocLikeFn to onlyAccessesInaccessibleMemory.

From my perspective, since the return value is noalias, I feel like that memory is inaccessible until it is returned so anything done to that memory, such as clearing it, should be fine. It seems like the issue that they had was with various bailouts for inaccessible memory in llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp. We do have a check for an allocation function in storeIsNoop. I don't know if that happens before the other checks but it seems at least worth testing to see whether the old issue still occurs on today's code: https://github.com/llvm/llvm-project/issues/49487. It probably wouldn't be hard to add a test for that

Jan 17 2022, 12:37 PM · Restricted Project

Jan 16 2022

Bryce-MW added a comment to D117180: [BasicAliasAnalysis] Switch from isMallocOrCallocLikeFn to onlyAccessesInaccessibleMemory.

If I can manage to find my way around the clang source code, I could put in a patch and see what people say about it?

Jan 16 2022, 12:37 PM · Restricted Project
Bryce-MW added a comment to D117356: InstructionCombining: avoid eliding mismatched alloc/free pairs.

I just wanted to say, personally I like this idea a lot. Even if there is a better way to solve the specific problem in https://github.com/llvm/llvm-project/issues/40384, this overall gives more information which may be useful in the future. This is also part of what someone from Rust was asking for on the llvm-dev mailing list (which was the email that got me into contributing at all). I'll leave it up to the more experienced devs whether to approve this patch since they have a better idea of what they want the memory builtins system to look like but I do think it should be approved.

Jan 16 2022, 12:26 PM · Restricted Project, Restricted Project
Bryce-MW added a comment to D117180: [BasicAliasAnalysis] Switch from isMallocOrCallocLikeFn to onlyAccessesInaccessibleMemory.

My current thought on how this could be done is to add some sort of optimization flag to clang, say -fstrict-op-new. If off (-fno-strict-op-new), it would mark op new as inaccessiblememonly and possibly removable if we ever add a way to mark that. When they would enable that flag would be up to them. I would say disabled unless -Ofast but maybe because of the regressions we've seen, it should be disabled at -O3 or something. I can't see any easier way to solve this issue unless we basically leave this optimization to LTO where we would know if op new was globally replaced or not.

Jan 16 2022, 12:19 PM · Restricted Project

Jan 14 2022

Bryce-MW added a comment to D117180: [BasicAliasAnalysis] Switch from isMallocOrCallocLikeFn to onlyAccessesInaccessibleMemory.

That's worse than I thought since it appears LLVM can't know if the allocator was replaced until link time it seems

Jan 14 2022, 4:09 PM · Restricted Project
Bryce-MW reopened D117180: [BasicAliasAnalysis] Switch from isMallocOrCallocLikeFn to onlyAccessesInaccessibleMemory.
Jan 14 2022, 3:22 PM · Restricted Project
Bryce-MW added a comment to D117180: [BasicAliasAnalysis] Switch from isMallocOrCallocLikeFn to onlyAccessesInaccessibleMemory.

I've reverted it. That's quite significant. I'm not sure what we should do here. The current situation is incorrect but as we've seen fixing it causes significant regression. I'm guessing even if op new can't technically be considered inaccessiblememonly in the general case, it probably is commonly inaccessiblememonly which is why this worked in the past. I'm not familiar enough with clang to know if it could say add the annotation if linking against a library that is known to have op new be inaccessiblememonly.

Jan 14 2022, 3:17 PM · Restricted Project
Bryce-MW added a reverting change for rG1f2cfc4fdc1e: [BasicAliasAnalysis] Remove isMallocOrCallocLikeFn: rGdd13744bfb0a: Revert "[BasicAliasAnalysis] Remove isMallocOrCallocLikeFn".
Jan 14 2022, 2:43 PM
Bryce-MW committed rGdd13744bfb0a: Revert "[BasicAliasAnalysis] Remove isMallocOrCallocLikeFn" (authored by Bryce-MW).
Revert "[BasicAliasAnalysis] Remove isMallocOrCallocLikeFn"
Jan 14 2022, 2:43 PM
Bryce-MW added a reverting change for D117180: [BasicAliasAnalysis] Switch from isMallocOrCallocLikeFn to onlyAccessesInaccessibleMemory: rGdd13744bfb0a: Revert "[BasicAliasAnalysis] Remove isMallocOrCallocLikeFn".
Jan 14 2022, 2:43 PM · Restricted Project
Bryce-MW committed rG1f2cfc4fdc1e: [BasicAliasAnalysis] Remove isMallocOrCallocLikeFn (authored by Bryce-MW).
[BasicAliasAnalysis] Remove isMallocOrCallocLikeFn
Jan 14 2022, 12:22 PM
Bryce-MW closed D117180: [BasicAliasAnalysis] Switch from isMallocOrCallocLikeFn to onlyAccessesInaccessibleMemory.
Jan 14 2022, 12:22 PM · Restricted Project
Bryce-MW abandoned D117265: [MemoryBuiltins] [NFC] Rename isAllocationFn.
Jan 14 2022, 12:18 PM · Restricted Project

Jan 13 2022

Bryce-MW committed rG28b6e2cb3df6: [Attributor] [NFC] Use canonical variable name (authored by Bryce-MW).
[Attributor] [NFC] Use canonical variable name
Jan 13 2022, 11:07 PM
Bryce-MW committed rG83338d503242: [MemoryBuiltins] [NFC] Add missing section comments (authored by Bryce-MW).
[MemoryBuiltins] [NFC] Add missing section comments
Jan 13 2022, 5:44 PM
Bryce-MW requested review of D117265: [MemoryBuiltins] [NFC] Rename isAllocationFn.
Jan 13 2022, 5:30 PM · Restricted Project
Bryce-MW abandoned D116971: [AttributorAttributes] Remove hardcoded parameters.

I'm abandoning this in favour of D117241

Jan 13 2022, 3:30 PM · Restricted Project
Bryce-MW committed rG68874d8b5f78: [MemoryBuiltins] [NFC] Remove unused overload of isAlignedAllocLikeFn (authored by Bryce-MW).
[MemoryBuiltins] [NFC] Remove unused overload of isAlignedAllocLikeFn
Jan 13 2022, 3:19 PM
Bryce-MW closed D117245: [MemoryBuiltins] [NFC] Remove unused overload of isAlignedAllocLikeFn.
Jan 13 2022, 3:19 PM · Restricted Project
Bryce-MW accepted D117241: [Attributor] Share code for abstract interpretation of allocation sizes with getObjectSize [NFC-ish].

I like this a lot better than my solution and it looks to be correct.

Jan 13 2022, 3:18 PM · Restricted Project
Bryce-MW accepted D117242: [Attributor] Generalize heap to stack to any allocator with relevant properties.

LGTM

Jan 13 2022, 12:30 PM · Restricted Project
Bryce-MW added a comment to D117180: [BasicAliasAnalysis] Switch from isMallocOrCallocLikeFn to onlyAccessesInaccessibleMemory.

Based on the comments on AllocType, it seems that the original difference between OpNewLike and MallocLike was whether it returned null. In that context, it's reasonable that something that never returns null can be used in the place of something that could return null. It's obviously used for more than just that now so this has probably caused a few places to be incorrect.

Jan 13 2022, 12:28 PM · Restricted Project
Bryce-MW added a comment to D117180: [BasicAliasAnalysis] Switch from isMallocOrCallocLikeFn to onlyAccessesInaccessibleMemory.

Thanks, like I said, I'm not super knowledgeable about the standards (especially for C++, I'm mostly a C person). One thing I will note is that isMallocOrCallocLikeFn does return true for op new as far as I can tell.
The test it does is

(FnData->AllocTy & MallocOrCallocLike) != FnData->AllocTy // returns true when they type doesn't match and false when it does
Jan 13 2022, 12:14 PM · Restricted Project
Bryce-MW added a comment to D117180: [BasicAliasAnalysis] Switch from isMallocOrCallocLikeFn to onlyAccessesInaccessibleMemory.

I'll see what happens when removing that section. It does sound like that should be added to clang. I'll take a look at that before this is landed. I don't know enough about the standards to say if op new is required to only access inaccessible memory but that sounds reasonable since I think op new is supposed to not be observable.

Jan 13 2022, 11:48 AM · Restricted Project
Bryce-MW updated the diff for D117180: [BasicAliasAnalysis] Switch from isMallocOrCallocLikeFn to onlyAccessesInaccessibleMemory.
  • Try removing the code
Jan 13 2022, 11:45 AM · Restricted Project

Jan 12 2022

Bryce-MW updated the diff for D117180: [BasicAliasAnalysis] Switch from isMallocOrCallocLikeFn to onlyAccessesInaccessibleMemory.
  • malloc is inaccessiblememonly
Jan 12 2022, 9:00 PM · Restricted Project
Bryce-MW requested review of D117180: [BasicAliasAnalysis] Switch from isMallocOrCallocLikeFn to onlyAccessesInaccessibleMemory.
Jan 12 2022, 7:27 PM · Restricted Project
Bryce-MW added a comment to D116971: [AttributorAttributes] Remove hardcoded parameters.

Thanks! I wasn't a big fan of getAllocSizeParams anyway. I'll wait to see how your new version works.

Jan 12 2022, 6:48 PM · Restricted Project
Bryce-MW updated the diff for D116971: [AttributorAttributes] Remove hardcoded parameters.

I've rebased onto your commits. I don't really understand how the size system you put in works so I'll just have to trust you. Hopefully we can remove getAllocSizeArgs from getSize. Using getObjectSize was causing errors in tests before but it would be good to remove this extra function. Thanks for those commits!

Jan 12 2022, 6:21 PM · Restricted Project
Bryce-MW updated the diff for D116971: [AttributorAttributes] Remove hardcoded parameters.
  • Rebase since test issue seems unrelated
Jan 12 2022, 4:58 PM · Restricted Project
Bryce-MW updated the diff for D116971: [AttributorAttributes] Remove hardcoded parameters.
  • Attempt to fix type issue
Jan 12 2022, 3:57 PM · Restricted Project
Bryce-MW added a comment to D116971: [AttributorAttributes] Remove hardcoded parameters.

I'm not totally sure why it it failing. Something about the memset parameters not being correct though I'm not entirely sure why. I'll look into it. I also forgot to confirm that this is intended for contribution and I agree to the license terms. I'll ask for commit rights after this. I think having a track record of a few commits is probably good.

Jan 12 2022, 2:30 PM · Restricted Project
Bryce-MW added inline comments to D116971: [AttributorAttributes] Remove hardcoded parameters.
Jan 12 2022, 12:56 PM · Restricted Project
Bryce-MW updated the diff for D116971: [AttributorAttributes] Remove hardcoded parameters.
  • Check initial value when needed
Jan 12 2022, 12:56 PM · Restricted Project
Bryce-MW updated the diff for D117084: [CFLGraph] Change isMallocOrCallocLikeFn to isNoAliasCall now that allocation functions must be marked noalias.
  • Use onlyAccessesInaccessibleMemory instead
Jan 12 2022, 12:03 PM · Restricted Project
Bryce-MW added a comment to D117084: [CFLGraph] Change isMallocOrCallocLikeFn to isNoAliasCall now that allocation functions must be marked noalias.

Thanks for catching that. I'm glad we have review. For a normal function, it tries interprocedural analysis, if that fails, it looks at the readonly and noalias attributes. I think that allocation functions can go through the same process but just also check Call.onlyAccessesInaccessibleMemory(). This would mean that allocation functions could undergo interprocedural analysis first which would be a functionality change from before.

Jan 12 2022, 12:02 PM · Restricted Project
Bryce-MW added a comment to D116851: [MemoryBuiltins] Add field for alignment argument.

There probably should be an allocalign attribute in the future once allocation functions are less hardcoded

Jan 12 2022, 11:58 AM · Restricted Project

Jan 11 2022

Bryce-MW updated the diff for D117084: [CFLGraph] Change isMallocOrCallocLikeFn to isNoAliasCall now that allocation functions must be marked noalias.
  • Rebase to fix failing test on Windows
Jan 11 2022, 10:22 PM · Restricted Project
Bryce-MW updated the diff for D116971: [AttributorAttributes] Remove hardcoded parameters.
  • Rebase to fix failing test on Windows
Jan 11 2022, 10:20 PM · Restricted Project
Bryce-MW updated the diff for D116971: [AttributorAttributes] Remove hardcoded parameters.
  • Allocation must be removable to put it on the stack. Currently, all allocations are but this may change for some languages.
Jan 11 2022, 9:41 PM · Restricted Project
Bryce-MW added a comment to D117084: [CFLGraph] Change isMallocOrCallocLikeFn to isNoAliasCall now that allocation functions must be marked noalias.

Hmm, I expected that Herald would add other reviewers automatically. I'm not sure if I can ask it to do that manually

Jan 11 2022, 8:56 PM · Restricted Project
Bryce-MW requested review of D117084: [CFLGraph] Change isMallocOrCallocLikeFn to isNoAliasCall now that allocation functions must be marked noalias.
Jan 11 2022, 8:55 PM · Restricted Project
Bryce-MW added a reviewer for D116971: [AttributorAttributes] Remove hardcoded parameters: reames.
Jan 11 2022, 8:01 PM · Restricted Project
Bryce-MW added a comment to D116971: [AttributorAttributes] Remove hardcoded parameters.

I am unsure why tools/llvm-libtool-darwin/L-and-l.test failed on Windows. It didn't fail before and seems completely unrelated. I've done a rebase in case that helps.

Jan 11 2022, 6:40 PM · Restricted Project
Bryce-MW updated the diff for D116971: [AttributorAttributes] Remove hardcoded parameters.
  • Rebase
Jan 11 2022, 6:26 PM · Restricted Project
Bryce-MW updated the diff for D116971: [AttributorAttributes] Remove hardcoded parameters.
  • Forgot the constant alignment check
Jan 11 2022, 5:29 PM · Restricted Project
Bryce-MW updated the diff for D116971: [AttributorAttributes] Remove hardcoded parameters.
  • Rebase
  • Try different getSize
Jan 11 2022, 5:27 PM · Restricted Project

Jan 10 2022

Bryce-MW updated the diff for D116971: [AttributorAttributes] Remove hardcoded parameters.
  • Possibly fix unknown alignment issue
Jan 10 2022, 5:21 PM · Restricted Project
Bryce-MW updated the diff for D116971: [AttributorAttributes] Remove hardcoded parameters.
  • Possibly fix unknown alignment issue
Jan 10 2022, 5:20 PM · Restricted Project
Bryce-MW updated the diff for D116971: [AttributorAttributes] Remove hardcoded parameters.
  • Possibly fix unknown alignment issue
Jan 10 2022, 5:13 PM · Restricted Project
Bryce-MW updated the diff for D116971: [AttributorAttributes] Remove hardcoded parameters.
  • Check for undef value correctly
Jan 10 2022, 3:50 PM · Restricted Project
Bryce-MW added a comment to D116971: [AttributorAttributes] Remove hardcoded parameters.

Based on the build results. It looks like there are some other issues. I'll try to debug them later today or tomorrow.

Jan 10 2022, 3:37 PM · Restricted Project
Bryce-MW added a comment to D116971: [AttributorAttributes] Remove hardcoded parameters.

At least for now, I'll leave in the getAllocSizeArgs function since I don't see another way to get the non constant size. If we can't handle non-constant alignment, what should we do with calls that have it? I suspect that's a situation that would be very rare so I would be fine just cancelling the conversion but I'm not sure how to do that inside of manifest. Maybe I can just change line 5967 to continue; but I want to check if I have to set something to invalid or something else. It seems like the old version just asserts that it's constant so I suppose I could do that as well but it doesn't feel right to me.

Jan 10 2022, 3:30 PM · Restricted Project
Bryce-MW updated the diff for D116971: [AttributorAttributes] Remove hardcoded parameters.
  • getAllocSizeArgs doesn't work with strdup
Jan 10 2022, 1:50 PM · Restricted Project
Bryce-MW requested review of D116971: [AttributorAttributes] Remove hardcoded parameters.
Jan 10 2022, 1:45 PM · Restricted Project

Jan 9 2022

Bryce-MW added a comment to D116851: [MemoryBuiltins] Add field for alignment argument.

I confirm that this is intended as a contribution to the LLVM project and that I agree to the license terms thereof.

Jan 9 2022, 6:57 PM · Restricted Project
Bryce-MW added a comment to D116851: [MemoryBuiltins] Add field for alignment argument.

I am assuming that I don't have commit access since this is my first contribution so I would appreciate you landing the changes outside of Attributor.

Jan 9 2022, 4:19 PM · Restricted Project

Jan 8 2022

Bryce-MW updated the diff for D116851: [MemoryBuiltins] Add field for alignment argument.
  • Format (other than the table as mentioned)
Jan 8 2022, 10:51 PM · Restricted Project
Bryce-MW added a comment to D116851: [MemoryBuiltins] Add field for alignment argument.

I've rolled back the addition of the size function and the changes to attributor that go with it (I'll put those together in a separate patch once this one is done). I've kept the changes to attributor that are related to alignment but I can remove those as well if you would like, I wasn't entirely sure if you meant for me to do that or not. Thanks!

Jan 8 2022, 10:45 PM · Restricted Project
Bryce-MW updated the diff for D116851: [MemoryBuiltins] Add field for alignment argument.
  • Rollback changes to Attributor and addition of size call
Jan 8 2022, 10:39 PM · Restricted Project
Bryce-MW added a comment to D116851: [MemoryBuiltins] Add field for alignment argument.

Thanks for the suggestions! I had a feeling that there would be a better way than passing indexes around. I think I've mostly implemented them correctly. I'm not entirely sure if my use of getInitialValueOfAllocation is correct. I am also not sure about the part where getAllocSize has the possibility of creating a multiplication. Maybe that should return a pair of Values and let the caller create a multiplication if it wants?

Jan 8 2022, 12:40 PM · Restricted Project
Bryce-MW updated the diff for D116851: [MemoryBuiltins] Add field for alignment argument.
  • Remove the last index-based check
Jan 8 2022, 12:35 PM · Restricted Project
Bryce-MW updated the diff for D116851: [MemoryBuiltins] Add field for alignment argument.
  • Return a Value and add a size call
Jan 8 2022, 12:28 PM · Restricted Project

Jan 7 2022

Bryce-MW added a comment to D116851: [MemoryBuiltins] Add field for alignment argument.

The AllocationFnData table was not properly formatted before my modifications. When running clang-format, it changed the formatting of the entire table. I formatted it back to how it looked before but fixed a few issues with it. I think this looks better and is easier to read but I am happy to set it back to the way clang-format likes it if that is desired. Let me know. I think that was the only issue with the build.

Jan 7 2022, 11:21 PM · Restricted Project
Bryce-MW updated the diff for D116851: [MemoryBuiltins] Add field for alignment argument.
  • Fix build issue
Jan 7 2022, 10:41 PM · Restricted Project
Bryce-MW requested review of D116851: [MemoryBuiltins] Add field for alignment argument.
Jan 7 2022, 9:35 PM · Restricted Project