User Details
- User Since
- Jan 15 2018, 8:31 AM (157 w, 3 d)
Today
I'd recommend committing the NFC changes before this, no need for a separate review.
Yesterday
Thank you for the patch, I definitely think this is a step in the right direction! However, I think we are still broken in common cases, the first of which can be seen by simply removing amdgpu_kernel from llvm/test/CodeGen/AMDGPU/stack-pointer-offset-relative-frameindex.ll and noticing that the soffset of the buffer_load_dwords in %bb.1 do not correctly refer to the frame pointer, they are constant 0 just like in the kernel case:
Sun, Jan 17
Wed, Jan 13
Mon, Jan 11
Fri, Jan 8
Wed, Dec 23
Dec 22 2020
I'm still working through the whole patch series (thank you for the careful breakdown of the changes and their dependencies, it has been a big help!) but I'm going to be out for a bit around the holidays and wanted to post at least some of the feedback I do have.
No other target overrides GetDefaultDwarfVersion with the same implementation, and in the future when we make changes we can add them at the points in the hierarchy where they make sense semantically.
Dec 21 2020
Dec 17 2020
Dec 16 2020
Dec 15 2020
LGTM, thank you!
The change makes sense to me, but at this point it might be good to add a comment for find_first_existing_vc_file to say what it does and what each possible return value means. Maybe something like:
Dec 14 2020
Dec 11 2020
This was our first approach, but we found other places which were sensitive to ExceptionsType being set. Maybe those aren't present anymore; is anyone aware if that option should affect CodeGen? I can take another look now, and see if I can point to the issues we had.
Delete UnwindStreamer, merging the behavior into DwarfCFIException. Also rename
the MCAsmInfo property to SupportsDebugOnlyCFI to be more direct/accurate about
the semantics.
Dec 10 2020
Small proposed addition to try to encourage the migration to the no-explicit-N form, let me know what you think.
Dec 9 2020
Rebase over changes which eliminate the spurious newlines in the assembly
output of the DebugInfo test.
Rebase the original version of the patch again; this is the same diff that was
approved already, so I am going to just go ahead and commit it. If anyone has
any objections I can open another review to address them.
OK, thank you both for taking a look! I didn't go and update all of the failing test cases at first, because I wanted to avoid it if we decided to go the "type: is optional" route. I'll update them all and post to the review.
Dec 8 2020
Rebase over https://reviews.llvm.org/D92084
Dec 4 2020
Alternatively I'm open to fix this in the caller if the (EndOfStatement, "\0") value is correct here. I just didn't see what use the caller would have for a string containing an embedded null here.
Merge new test into existing cfi.s test, remove gratuitous directives in the
round trip test. Replace use of deprecated FileCheck syntax.
Dec 3 2020
Nov 20 2020
I somehow managed to omit deleting the old tests in the last patchset. I think it was implied that I would, but I'm pushing up to make sure there aren't any objections (and I didn't make any mistakes).
Nov 18 2020
Remove one redundant call to vector::shrink_to_fit
I also added calls to vector::shrink_to_fit to maybe make it slightly more likely for a sanitizer without c++ STL annotations to catch out-of-bounds accesses, but if this is just noise I'm happy to remove them.
I added something along the lines of what you proposed, let me know what you
think. I also switched to a vector<uint8_t> to get more explicit control over
the terminator, and to yaml::Stream to be able to directly call validate()
such that every document is checked (in case the fuzzer produces inputs with
multiple documents).
Nov 17 2020
Opened by mistake, intended to update D91545
Split up tests on combinations of addrspace, memory scope, memory ordering, and operation. I felt this was a reasonable amount of tests, but I can split things down further if needed.
Nov 16 2020
I ran the fuzzer and quickly found two places I had missed. I've been running the fuzzer for about a half hour now and haven't seen another crash, so I am tentatively posting the patch. I'll leave the fuzzer running for as long as I reasonably can to try to find additional cases.