- User Since
- Aug 6 2019, 8:30 AM (216 w, 4 d)
Feb 24 2020
[PHIEliminate] splice dbg instruction when LowerPHINode
DBG_VALUEs between PHI and LABEL will block PHI lownering after LABEL, move all DBG_VALUEs after Label before PHI lowering.
Feb 14 2020
ok, I'll update the patch later.
Jan 17 2020
My proposed code is as below: moving debug instrs after LastPHIIt at the beginning of PHIElimination
Jan 16 2020
We try to move DBG_VALUEs after PHI and LABELs while ScheduleDAGSDNodes on early stage, but there is the the case the DBG_VALUEs exist between PHI and LABELs after Later PASS. Looks like we need to move DBG_VALUEs after LABELs on the beginning of PHIElimination.
Checked with extracted.ll, on the block : the dbg.value did not skip "llvm.lifetime" and "%21 add" move after Label by SkipPHIsAndLabels() while ScheduleDAGSDNodes.
And later PASS (after ScheduleDAGSDNodes) removed "call llvm.lifetime.start" and "%21 add", and then left DBG_VALUEs between PHI and LABELs, and make errors.
Jan 8 2020
Jan 7 2020
update patch with:
- rename dbg_values as DBG_VALUEs to keep consistent.
- add MBB.empty check at the beginning of MachineVerifier::checkPHIDbg
Dec 17 2019
fix code style issue.
Added checkPHIDbg to check "no dbg_values between PHI and labels" in MachineVerifier.
Look like adding one more method for SkipPHIsAndLabels with const_iterator would be more easier. I thought will be more duplicate code to implement SkipPHIsAndLabels and SkipPHIsLabelsAndDebug in MachineVerifier.
As the "No debug in PHI and labels" assert already be added in PHIElimination.cpp, is that safe if we do not add the checking in MachineVerifier?
Dec 13 2019
Just updated the code [Diff 233770] follow the comments by jmose and bjope.
I try to add below reference code to MachineVerifier::checkPHIOps, but meet a build error. In the function the type of MBB.begin() is const_iterator, but type of SkipPHIsAndLabels argument is (iterator I). How to convert const_iterator to iterator and pass to SkipPHIsAndLabels? Could I write the MachineVerifier as below code?
Dec 12 2019
@bjope, thanks for comments, that's a good idea, better than remember "No debug in PHI and labels" in mind.
Base on the solution "Postpone insertion of CFI instructions (pull out - reinsert)", with the patch, the CFI Instruction has been pulled out, but the problem is need to find the "correct position" to re-insert the CFI Instruction.
Dec 11 2019
Dec 6 2019
description for updated revision:
After study the history of the PHIElimination code changes, finally, I found this issue had been fixed before, and later the fixing code had been removed when created the SkipPHIsAndLables as new helper.
Dec 4 2019
Another idea is to add one more helper like getFirstNonPHIsAndLabels(), replace getFirstNonPHI in ScheduleDAGSDNodes.cpp#L935.
Insert dbg_values after PHIs and Labels, so that there will be no dbg_values barrier between PHI and LABELS.
I try to move DBG_VALUE after COPY, but have some doubts:
- Is there the case that DBG_VALUE be exist after LABELs use %1 definition? if there is, we need to move all DBG_VALUEs, if not, we just need to move DBG_VALUE which located after PHIs.
- After refactoring by using AfterPHIsIt instead of LastPHIIt, there is the case that the basic block only have two PHIs, AfterPHIslt will be null and pass to LowerPHINode(), not sure if passing argument(null) to a function is safety or not?
Nov 25 2019
Updated the code and here is the description:
Nov 22 2019
update description in test case
Nov 10 2019
bb.0:TEST8rr killed renamable $al, renamable $al, implicit-def $eflags JCC_1 %bb.1, 5, implicit killed $eflags
@bjope thanks for the information. Without this patch, I have seem some check issues already there, I suggest to fix the issue first before apply this patch, than verify this patch. I don't know whether the CHECK in test case is correct or not, would you please help to provide test case without debug, help me find the normal behavior that could pass llc check without error, so that we could compare the result and diagnose why debug or cfi instructions impact the branch-folder pass. As this PR already merged, It's better for us to file a new bug and fix with another commit.
Oct 30 2019
Look at the debugging printf information, I found the begin() is the first instruction of the BasicBlock, but the end() is not the last instruction, it's something unknown or none flag.
If what I found is correct, and would say end() could not be debug instruction, and maybe we needn't consider about debug instruction impact for end() part.
Will it be better to use getFirstNonDebugInstr() instead of begin() on line 1475?
Oct 29 2019
@apilipenko thanks for accepting, I have no submit permission, would you please also help me to submit this patch? thanks.
Oct 28 2019
@bjope,@jmorse, @aprantl, @jdoerfert, @vsk thank you all for kind and patient help on code review and giving me many supports and guidelines, I'm glad this(my first) patch be accepted and merged on llvm-project. Really thank you.
@bjope thanks for help, to be honest, I don't know the process or execution of commit, I thought I have no commit permission right now.
Current status is "accepted and ready to land" for a long time.
I'm new on LLVM, Is there anything I need to do to make this PR be merged?
Oct 20 2019
update test case:
- using function name @foo and @bar instead of @f and @g for more complete match
2.cleanup test case with "-strip-dead-prototypes -strip-dead-debug-info"
Oct 18 2019
just added use_empty check in the inner loop, this will be the smallest code change for issue fixing.
Oct 16 2019
Oct 15 2019
fix typo of patch description
Rethink about this issue and the root cause should be caused by use_empty. Just resubmitted the patch with "[InlineFunction] Erase use_empty allocas when inline".
- Remove the check for use_empty in the outer loop.
- Add a check for !use_empty in the inner loop.
- Remove the inner loop (i.e only splice one alloca at a time).
Alternative 3 would be the simplest. If there really is a speedup on doing fewer splices, then alternative 2 still moves consequtive !use_empty allocas in batches. The idea with alternative 2 is to split batches on allocas that has no uses (as they are handled in the outer loop). Alternative 1 might work assuming that allocas with no uses are cleaned up somewhere else. But I think this alternative is the least interesting one.
Oct 11 2019
So the root cause is rather that we treat an alloca being immediately preceeded by another alloca differrently from the case when it is preceeded by another kind of instruction. This happens also when having other instructions in between, and is not specific to dbg intrinsics (could be interesting to add a test case where you replace the dbg intrinsics by something else).
Yes I think so, if the other instruction is not dbg instr which exist between two allocas, the InlineFunction with and without "-strip-debug" will make the same behavior, that should both erase second use_empty alloca. This patch is to fix the issue that debug instr impact InlineFunction generate different output.
Oct 10 2019
- Use BasicBlock::iterator skipDebugIntrinsics instead of rolling directly. The recommend API BasicBlock:: instructionsWithoutDebug() looks not very suitable to handle llvm::Instruction in this code.
- follow llvm style, condition: break first, then unconditionally execute the code
[inline] skip dbg info when scan allocas
Oct 8 2019
just added updated test, could some one help to review again? thanks.
Added one more RUN command line with "-debugify-each" in existed call-guard.ll, needn't duplicate another same test case file.
Sep 25 2019
thanks for review. add test case with --debugify-each based on previous call-guard.ll
Sep 24 2019
[InstCombine] Fix call guard difference with dbg
Sep 2 2019
update branch-folder-with-debug.mir follow the review comments
- update CHECK block lines before successors check
- Add "CHECK-NEXT" for successors after block
- Simplify address after successors block
- move RUN line to top and add more descriptions.
Aug 27 2019
@djtodoro thanks for review, followed your comments and guide from https://llvm.org/docs/MIRLangRef.html#id14, I have simplified mir test code by removing all unnecessary code for FileCheck.
"llc -o - test.mir -mtriple=x86_64-- -run-pass=branch-folder | FileCheck test.mir"
Simplify MIR test case
Aug 26 2019
add branch-folder-with-debug.mir test
Aug 21 2019
Please allow me to add Hsiangkai and Petar as reviewer, as you had contributed related patches about debug and CFI instruction skipping. Thanks.
thanks for review and comments. While adding test code, found another error about "unnamed alloca" before branch-folder by llc, I'll investigate the issue before submit the test code.