- User Since
- Nov 16 2017, 6:22 PM (13 w, 4 d)
Thu, Feb 15
I'm happy to take a look at cleaning up the isLoweredToCall(). Taking a hint from the comment in that routine, I think it makes sense to move that logic to another class, perhaps the base TargetLowering class, after all, this routine is about lowering.
Wed, Feb 14
Great - can you commit this yourself or would you like me to do it for you?
Thanks @dblaikie! I renamed the test, and cleaned up per your suggestion. I originally regex'd the debug-info lines so that the test would verify that the names were artificial; however, being that we already match them as metadata earlier, it's not really that necessary; we are only testing name strings anyways. Thanks again for the suggestion.
- Added a division by 2 instead of the shift, it reads clearer.
- Updated the associated comment, reflecting that we divide by two because the variables we are annotating are within the inner scope of the ranged-based for loop.
Tue, Feb 13
@davide, thanks. I am interested to see what you all find out.
@dberlin, thanks for the comment. I do not have numbers for any of the cases where LCSSA is known to be a bottleneck, but I do see your point. Even on a small test case it is one of the more expensive passes, but that sample is probably too small to make any judgement upon.
@vsk thanks! I was referring to a "requested change" on an earlier version of this patch, which (I think) is preventing this current change from showing up as "Ready to Land."
Ping. Is there anything else we need to do here? I think the remaining approval was due to the original patch being rejected because it did not cover the catch/landing pads, prior to using the getFirstInsertionPt().
Fri, Feb 9
Thu, Feb 8
Thanks for the test tips. I realize the original was a bit carried away, my apologies for that. This updated test visits the same code path that we're trying to test, and is much more concise.
Thanks for the look @rjmccall . I tossed in the shortcut as you suggested. I also had to fix the instruction numbering in the test case.
Tue, Feb 6
- Removed function comments and lifetime intrinsics from the test case.
- Use isEHPad in place of the explicit CatchSwitch instruction check.
This is an updated patch to handle the insertion of dbg.value intrinsics on behalf of LCSSA PHI generation. The original logic was reverted due to a dbg.value instrinsic attempting to be inserted into a catchswitch block. According to LangRef, catchswitch are to be the only non-PHI instructions within a block:
"The catchswitch is both a terminator and a “pad” instruction, meaning that it must be both the first non-phi instruction and last instruction in the basic block. Therefore, it must be the only non-phi instruction in the block."
Mon, Feb 5
Thanks @djtodoro! LGTM
Fri, Feb 2
Thu, Feb 1
Updating the diff, missed a few deltas that should have been in the original patch.
@djtodoro thanks, this change looks nice, it makes the code easier to read too!
Just a suggestion, could we just set the Subprogram earlier where we set all of the
more basic properties of the function, when NF is created around line 819, or is
that too soon?
Mon, Jan 29
Fri, Jan 26
Thu, Jan 25
I just ran a test on my end using getFirstInsertionPt, looks good, but no clue what greendragon will say.
Additionally, I'm not sure insertingAfter would be correct either. It seems we might need a check for the cases that @davide mentions.
Thanks for the approval @vsk. I do not have commit access, so I'll need someone to commit this on my behalf. Should I remove the paranoia- check you noted?
Hi @djtodoro, thanks for looking into this. I have a few comments/suggestions noted inline.
Thanks @majnemer! Would you mind committing this on my behalf? I do not have commit access, thanks.
Tue, Jan 23
Ping. Since I don't have commit access I'll need someone else to commit this on my behalf.
Mon, Jan 22
Jan 18 2018
I'm certainly fine with always allowing this pragma. I can see it useful for other editors that might wish to utilize the hint.
Jan 12 2018
I'm not certain how helpful this change is, per some discussion on the list last week, this change implies that linkers can also support extremely large sh_entsize values. The intention with this patch is to represent the same sh_entsize size used in elf.h for 64bit elfs.
Changes have been committed at r322305.
Jan 11 2018
Thanks for the suggestion @hfinkel, and I agree, using the isLoweredToCall predicate seems the most comprehensive solution; however, I did find it a bit more intrusive than I was expecting. As a side-note, I slapped together a much simpler/less-intrusive patch, that only special-cased the dbg intrinsics and not checking the lowering information, but I feel using the lowering data is more correct.
I do need someone to commit this on my behalf. Thanks!
Jan 5 2018
Thanks Paul, I like that suggestion.
Jan 4 2018
Jan 2 2018
Abandoning as this is no longer a problem with release builds.
Ping :) I would like someone to commit this on my behalf, thanks!
Dec 19 2017
Thanks Eli! I do not have commit permissions, so someone else will have to commit this on my behalf.
Thanks for the suggestion Eli. Took your initial suggestion.
Dec 17 2017
We probably want a new attribute to express the relevant property, which is essentially that most intrinsics don't call any function defined in your program.
Dec 12 2017
Thanks for the feedback Hal. I agree, I think many intrinsics can be added/marked-norecurse. But for this change I just wanted to isolate the change to the debugging intrinsics, and keeping with the theme that dbg intrinsics should not influence code generation.
Dec 11 2017
Dec 10 2017
Dec 6 2017
Dec 4 2017
Dec 2 2017
Thanks for the comments Eli! I agree, it makes sense to flag certain expressions to be type dependent at the earliest time of compilation. Clang already does this, as the Value/Type dependence is established upon Expr construction from MemberExpr construction.
Dec 1 2017
- Added a unittest to StringMapTests.
- Changed a use of unsigned to be size_t, when constructing a StringMapEntry.
Nov 30 2017
Nov 29 2017
Thanks for the approval Aaron. I do not have commit access, would you mind submitting this on my behalf?
- Removed the new lines.
- Added white space between the data type and pointer character.
- Updated the test to check the exact bounds, and over-bounds cases
- Combined the diagnostic messages into one via 'select'