- User Since
- Jan 15 2018, 8:31 AM (140 w, 6 d)
Aug 19 2020
Aug 17 2020
LGTM, and can you please open a follow-up review once this is committed to hide all of the Object::get<Target><Stuff> methods?
Aug 13 2020
I don't know that my opinion holds much weight, but I agree that reducing the number of nearly-equivalent-but-subtly-different intrinsics/pseudo-instructions for debug-information would make it easier for a new dev to understand their semantics, and make it much easier to extend. I would argue that using different representations based on complexity doesn't make the result any more comprehensible, it just adds more things to comprehend.
Aug 12 2020
Looking good to me! Some small nits, and it would be good to hear back on what the general consensus on adding this to Object is.
Aug 7 2020
Aug 6 2020
Aug 5 2020
LGTM, thank you!
Aug 4 2020
The other option is to treat any frame that can possibly have an offset that won't fit in the immediate field as needing an SP
I don't know if this has anything to do with David's issue, but I think separately this makes sense. I'm just confused what the semantic difference between the two "do we need an SP" predicates is?
Jul 31 2020
Jul 30 2020
Sorry for the insane delay, this makes perfect sense to me.
LGTM, thank you!
Jul 29 2020
Jul 28 2020
Jul 27 2020
Jul 24 2020
Jul 23 2020
I discussed with Tony today, and I was thinking about this the wrong way.
Can we just add a predicate for whatever necessitates the SETREG for gfx10+? No idea on a good name, but if it were flatScratchIsHwreg() and it implied flatScratchIsPointer() this becomes:
Jul 21 2020
Jul 16 2020
Jul 9 2020
Jul 8 2020
In an ideal world, we'd merge all the binary tools (GNU and LLVM) into a single tool, or redistribute functionality somehow, so that we don't have duplicate functionality like we already do. This takes us further away from that ideal.
Jul 6 2020
Jul 1 2020
Sorry for the delay in reviewing, I've tried to wrap my head around this and I'm still lost.
Jun 26 2020
Jun 24 2020
Jun 22 2020
Jun 18 2020
Jun 17 2020
Jun 12 2020
Jun 11 2020
LGTM, sorry for the delay in reviewing. My feeling is that we should just keep the simple rules you have outlined in the docs and require the callback implementation ensure things are not decoded twice by buffering their output (if needed) and setting Size correctly.
Jun 5 2020
I don't know why I didn't add any tests of getDesc() (or anything else) originally, maybe because there is no existing testing and it requires both a unit test and a binary file input? I think at this point we shouldn't block adding an obvious 1-line function on adding tests that should already exist. So, LGTM with Matt's request addressed, and we can add testing in a general way at some point.
Jun 4 2020
Jun 2 2020
I'm not comfortable accepting this as I'm not familiar with the code, but it LGTM. The pattern of constructing the MachineFunctionInfo lazily on the first call to getInfo seems very non-obvious; it isn't implied by the name and it isn't described in the doc comment. As you note it is also particularly dangerous if it depends on values which are mutable over the life of the MachineFunction.
May 28 2020
May 27 2020
May 21 2020
LGTM, thank you for bearing with us taking a while to review. I left a small request but feel free to commit without updating the review.
May 20 2020
I posted a piglit test to try to exercise this https://gitlab.freedesktop.org/mesa/piglit/-/merge_requests/291
May 19 2020
May 15 2020
I'd be interested in the answer concerning why we need to avoid git rev-parse HEAD; it seems like the cleanest solution is to just always check if git rev-parse HEAD changes to determine whether to regenerate the header.
May 14 2020
Correct test file names. I missed this when renaming the CFA operation itself
to match the updated proposal.
Use llvm-readelf for more compact tests
- Use -COUNT-N form
- Use '#' comment character
- Use llvm-readelf for more compact tests
- Refactor lit config test
Refer to subreg index directly
This will require some changes to the CFI, although previously this case was broken anyway so it does not regress the unwind information. I think we will just need to define the CFA relative to the base pointer (if it is actually set up) or the stack pointer (if realignment is needed, but the base pointer is not).
Re-add a test that got lost in a rebase somewhere
May 13 2020
Add tests for parse errors, fix formatting
LGTM, although CHECKs in the tests to confirm that e.g. the first involves a scavenged register and s_add, and the second involves the stack pointer and s_mov, and that this happens at the expected offset might make it easier to read and more likely to catch something that breaks this in the future. Right now it isn't clear why there are two nearly identical tests.
Rebase over https://reviews.llvm.org/D79878
May 12 2020
Rebase, generalize the previously hard-coded list of caller-saved registers
with help from @cdevadas, and emit CFI for the new case where we spill the FP
Rebase and update summary
Rebase and move tests to .debug_info as some new operations are not legal in
May 11 2020
Dinesh is working on merging metadata in ROCm CompilerSupport using MsgPackDocument, and it fell out that merging YAML metadata was also handled transparently. What is the advantage to doing this via a function pointer passed to readFromBlob? It doesn't seem to save any copies as you still need to be able to compare the nodes; is it just a matter of readability? Would it be reasonable to also add a callback for fromYAML?
May 8 2020
May 6 2020
Thank you for the explanation; I'm also still lost as to why .git/logs/HEAD is referenced at all then. I would propose removing find_first_existing_vc_file entirely, as it seems to be completely redundant if there is another mechanism for ensuring the VCS header is up-to-date without triggering gratuitous rebuilds, but I will defer to any of the other reviewers on the blame for the scripts.
May 5 2020
Can you provide more context in the diff? Looking at the current master locally it seems like this patch changes the behavior of the llvm_vcsrevision_h target when logs/HEAD isn't found, such that it only depends on the generation script. I.e. if you configure CMake without the HEAD file present, and then you do something which changes the working directory and creates HEAD the target will not be out of date and won't be rebuilt. Before this patch it would have noticed the change to HEAD and forced regenerating the version header.