- User Since
- Dec 25 2018, 11:14 PM (108 w, 2 d)
Sat, Jan 16
Dec 21 2020
Dec 16 2020
As this review has last so long, (1 month), It has accepted before and I fixed all reviewers' comments except @vsk 's "merging" suggestion.
I want to let this patch in first.
About @vsk 's "merging" suggestion, I am almost sure that the code will be some ugly compared with current code,
So I'll use another special patch to handle his "merging" suggestion, let/wait @vsk to accept it or not.
Dec 15 2020
Imo this overstates the difficulty of code sharing in this context, and underweights the potential long-term effects of having divergent code paths that are supposed to do the same thing. I urge you to reconsider :).
Hi @vsk, I am fine to merge if you still feel the following explanation doesn't hold water：
- The logic here not 100% same, currently, mir-check doesn't check the phi node. (which will eliminate at back end)
- I still feel it is trouble to "Distinguish isa<DbgValueInst>(&I) with MI.isDebugValue() and so on"
- In my eyes, the small logic of ir/mir check here is just happen to be same/similar, not must (or no need) to be, make them not affect each other is better.
Hello @vsk ,could you help accept it ?
Update affected tests: AArch64/GlobalISel/constant-mir-debugify.mir and phi-mir-debugify.mir
Because this patch add metadata "llvm.mir.debugify", it affected the index of some old test.
For e.g. change Checking "debug-location !10" to Checking "debug-location !11"
OK, thanks for reminding.
Dec 14 2020
Yes, it cause fail in the tests of D78135, I'll update it. thank you.
Dec 3 2020
Dec 2 2020
Dec 1 2020
Mark TODO for DBG_INSTR_REF and refine docs/HowToUpdateDebugInfo.rst
Nov 30 2020
@LuoYuanke I see you give the most comments, could you +1 for this patch. tks
Nov 28 2020
Nov 26 2020
Nov 25 2020
Nov 24 2020
Thanks very much for your reviews!!
Nov 23 2020
Nov 20 2020
Nov 18 2020
Nov 17 2020
All done, @djtodoro thanks very much for you review !
Nov 7 2020
Nov 3 2020
Then you can change your test case name to pr48064.mir
OK, good, I'll change it in committing.
I created a bug ID (48064) for it https://bugs.llvm.org/show_bug.cgi?id=48064
Nov 2 2020
Thanks for your review !
Hello @thanm, I think you care about this patch/bug, do you have additional comments? If you feel my idea is reasonable, could you help accept this patch, thank you again!
Oct 28 2020
First, I thanks for your careful reviewing.
The windows Exception model has work well for many years, I tend not to change it. If we carefully watch the catch values (e.g. Type * *exp.i), we will find that they usually have a very long life range guarded by lifetime.start and lifetime.end, (usually almost equal with the whole function life range).
This seems that the front/mid ends don't want to do much optimization on this special mechanism, because in their eyes, the back-end optimization should based on the lifetime.start and lifetime.end they generated.
And if we reflected it in the IR, it will bring much complexity into current clear mid end IR, because every function in try block (may contain branch) need to reflected that it may change the catch values (e.g. Type * *exp.i).
I also find that, current "-stackcoloring-lifetime-start-on-first-use" didn't handle the multi-lifetime.start/end case, catch value may has multi-first use. it is make sense to handle the corner case of catch variable in this option.
What's more, current patch is simple and clear, I think it is the most probably easiest way to fix this bug.
Oct 27 2020
Unlike linux, usually by @__cxa_begin_catch ,windows Exception didn't has a suitable port to reflect how it change the catch value, it is more like a "ABI" action, where the throw impliedly write and where the catch block get it.
I think the early designer do this mainly because the catch value (e.g. type** exp.i), is only used in adjacent catch block.
Oct 26 2020
Oct 25 2020
Oct 24 2020
Oct 21 2020
TKS all review!!
Oct 20 2020
Oct 19 2020
Oct 18 2020
Oct 15 2020
TKS for reviewing!
Oct 12 2020
Thanks for your reviews! Please let me suspend this patch first.