- User Since
- Mar 5 2019, 4:12 AM (211 w, 3 d)
Wed, Mar 22
I believe there are a few places in the existing codebase where we intentionally drop fragments that could be updated to use this class; on the whole though, I approve of using a new class to differentiate between a variable fragment that happens to be the whole variable vs an explicit reference to the whole variable regardless of fragmentation.
Tue, Mar 21
Simple enough, LGTM.
Wed, Mar 15
Added a test to check that --target-run-args and DexCommandLine compose - needs to be a separate test file because of a few necessary differences in the file, which can't be fixed by using sed in the RUN line.
Stack LGTM, seems like a quite straightforward set of changes.
Tue, Mar 14
Thu, Mar 2
LGTM - I think not deleting debug intrinsics if there would be no other codegen changes is the right call for now as a simplest viable fix for the linked issue; hopefully this doesn't result in intrinsic bloat, but if it does that can be solved separately.
Wed, Mar 1
Tue, Feb 28
Broadly looks good, just a couple questions and a nit comment.
Does anyone have any good ideas on what a test for this looks like?
Mon, Feb 27
Thu, Feb 23
Small suggestion below, but LGTM with or without it.
Feb 21 2023
Feb 10 2023
Thanks for the changes, LGTM.
Feb 8 2023
Nit, otherwise LGTM!
Looks broadly good to me, with some small suggestions.
Feb 7 2023
Actually looking back over it I'd add further - FindValueForBlock isn't necessarily right because we're looking for values in the middle of the block, and that function will give us the value at the end only (which may be incorrect depending on the position of the debug value). I encountered this problem in MachineSSAUpdater, and the approach I took there was to add a ExistingValueOnly parameter to GetValue(InMiddle|AtEnd)OfBlock that performs all the same logic but returns empty if it would need to generate any code to provide a value. Happy to repeat that fix here!
Feb 1 2023
Jan 19 2023
Jan 16 2023
Jan 11 2023
Small point about the CallBr guard, but no other issues from my PoV!
Jan 10 2023
Jan 9 2023
Jan 6 2023
Rebased onto latest main, added unit tests for new convertToUndefExpression, and converted some existing tests to test for the new fully-undef debug values that SelectionDAG produces.
Rebased onto latest main, slightly updated and added unit tests for convertToNonVariadicExpression.
Rebased onto latest master, added unit tests for convertToVariadicExpression.
Rebased again now that the patch stack is ready to land.
Address comments (extracted MO selection for const debug operands, expanded comments in EmitDbgValue), add a function for setting a DIExpression "undef".
Jan 5 2023
LGTM, thanks for the changes!
Jan 4 2023
Minor comments but otherwise LGTM.
Simple and good, LGTM.
LGTM with the suggested "changes".
Simple enough, LGTM (but since this is part of a more substantial patch stack, leave a little time for others to have a look).
Largely LGTM, but with a potential error that needs looking at.
Jan 3 2023
Rebased onto latest master; addressed review comments. The section of code that turned variadic expressions into non-variadic expressions so that they could be recovered as entry values has been extracted into a separate function in DebugInfoMetadata, and another instance of that logic in AsmPrinter has been updated to use this function. If you think that change should be rolled into a separate patch then I'm happy to do so!
Dec 20 2022
Updated for review comments, None -> std::nullopt.
Dec 19 2022
Dec 16 2022
Not very familiar with this area, but it looks as though currently InstCombinerImpl::transformConstExprCastCall in InstCombineCalls.cpp will need updating as well. Specifically it may transform a CallBr instruction and insert a Cast for the return, for which it calls NewCall->getInsertionPointAfterDef() and asserts that the return is non-null. Not sure what the solution would be, but it might require dropping support for CallBr in transformConstExprCastCall if the return type is changed: there's already a block in there that bails out of transforming invoke or callbr instructions if they are used in a PHI node, since there's no place to insert the Cast in that case.
Apologies for taking a bit of time to get to this. My thoughts below, tl;dr it's probably better to just skip tracking spills for DBG_VALUE_LISTs in RegAllocFast.
Dec 15 2022
Some notes about the DIArgList stuff, otherwise LGTM.
Dec 8 2022
Nov 23 2022
LGTM with minor comments.
Nov 22 2022
Nov 21 2022
Nov 17 2022
Changes all LGTM, except the one unaddressed comment about not using $noreg in loop-sink.ll.
Nov 16 2022
Nov 10 2022
Nov 7 2022
Move some changes from an earlier patch into this one: Make entry value expressions containing only a leading DW_OP_LLVM_arg, 0 valid, as the changes in this patch enable such expressions. Also add the convertToVariadicExpression function, as this is the first patch in which it is used.
Rewrote the test to remove some incorrect behaviour; new test uses two different variables to compare direct/indirect and non-variadic/variadic separately. Also, add the missing variable equality check in the added MachineInstr function (the only caller already checks for variable equivalence, but the function would be misleading if it didn't do so as well!)
Address review comments, including removing the unnecessary call to MachineInstr::isIdenticalTo in MachineInstr::isEquivalentDbgInstr and also adding a missing check for getDebugLoc equality.
Nov 4 2022
Some minor comments, but otherwise LGTM.