User Details
- User Since
- May 9 2013, 11:10 AM (402 w, 4 d)
Today
Fri, Jan 22
It's possible that the InstantiationGroups result is not supposed to be empty; in which case, something deeper is wrong, and I can abandon this patch and file a bug. Let me know.
Thu, Jan 21
I'd expect the pace of preparing the release to be basically the same regardless of start date, so the milestones would make more sense as start date plus N weeks.
If you want wiggle room for the start date, that's fine, anything that expresses "late January" and "late July" will work. I thought it was going to be more definitive, what with specific week numbers and all.
I think "Week number" is too ambiguous to be a guide. If January starts on the last day of the week, does that still count as week#1? What day does the week start on, anyway--much of the world starts the week on Sunday, much of the world starts the week on Monday.
"Fourth Tuesday in January/July" is unambiguous and makes everything easier to plan. Using "Start + N weeks" for the rest of the target dates is fine.
Tue, Jan 19
Catching up on this, sorry...
Mon, Jan 11
Wed, Jan 6
Tue, Jan 5
This version of the patch avoids the weirdness I was seeing with prolog instructions in certain cases.
Change how PHIs are handled; if the operand has a debug location, use it, otherwise don't set a debug location. Then, flushLocalValueMap() will look at the first local-value instruction; if it doesn't already have a debug location, we copy the location from the first instruction after the local-value instructions (which should have the debug location of the original IR instruction).
Dec 16 2020
Thanks Duncan! I (or someone) will play around with that and see what we need to do. May take a little while, as we're about to freeze a release and then go on break for two weeks, but good to know there's a straightforward path.
One thing this patch does, is make decisions about default behavior static. Meaning, the option behavior cannot depend on other options; specifically, it can't be based on the triple, which allows target-specific customization. PS4 certainly has cases where our defaults are different from the usual ones, and I'd kind of think that was true for other targets as well.
Dec 14 2020
I've run this through our copy of the GDB suite (8.3, not sure how old that is). There are 10 differences, with and without the patch.
Dec 10 2020
This is still showing as approved; I'm going to try "Request Review" to see if that helps.
Rebase and combine cf1cc774d and dc35368c, plus revise handling constants materialized due to PHIs.
I plan to do some build-time and object size measurements, similar to the first time around.
I plan to run this against the gdb test suite, or at least the local copy we have here at Sony, and post my findings.
Reopening, will upload a new diff in a moment that modifies how PHIs are handled. I intend to run the gdb suite on this, will post findings when I have them.
Dec 9 2020
Dec 8 2020
And with these changes together, breaking on the function breaks on line 5 (presumably because this is creating the constant used by the second '&&' which is on line 5) instead of line 3. Not the worst thing, but I imagine given Sony's interest in less "jumpy" line tables, this might not be super desirable.
It's breaking on an xorl %eax,%eax which is produced by the PHI at the end of the conditional expression, which now has the source location of the operator at the top of the expression tree, which is the second && operator, yeah. Not the best. (FTR, the jumpiness complaints we get are usually more egregious, hopping between different source statements somewhat arbitrarily; not sure anyone would complain too loudly about hopping around within an expression. We see less of that in any case, because we suppress column info, but still.)
Dec 3 2020
Sometihng like this seems plausible to me:
Dec 2 2020
Just for grins, change the && to || and see what happens...
The xorl becomes a movb $1 (no surprise there). But, that instruction no longer has line-0, instead it becomes part of the prologue.
This tells me that the xorl had an explicit line 0, while the 'movb $1' has no location (a subtle but important difference).
@rnk is correct that the phi's source location is propagating to the xorl. I modified the .ll file to put a different line number on the phi, and the xorl picked it up. This is clearly on the front-end to get right.
The constant materialization (xorl) obviously does not belong in the prologue; I think the instruction ordering with the patch is preferable.
But, clearly we'd rather not have it attributed to line 0, but the line it actually belongs to. I'll start looking into this. Offhand the reduced example appears to be not unreasonably large :-)
Dec 1 2020
The patch deletes llvm/CMakeLists.txt, which is surely not something you want to do.
Nov 30 2020
The tests all pass, but I wasn't *quite* daring enough to commit this without a second opinion.
Nov 25 2020
Abandoned in favor of D91734.
Nov 20 2020
Nov 19 2020
I have to say, I'm not super enthused about this feature. Typically we've added directives when there's no other way to achieve the desired effect; the LITERAL feature doesn't meet that bar. It's a way to simplify something that you could do already. Not to say I'm going to insist on killing it, but I would like to hear more justification.
I don't reproduce the HWASan failure on my Linux.
Fix an lld test and two lldb tests.
FTR, I plan to remove the -fast-isel-sink-local-instructions option. I think there might be some now-unused fields as well, such as LastFlushPoint, that could be removed. These are NFC follow-ups that can be deferred, this patch is big enough as it is.
Nov 18 2020
Some sort of pilot error on that first diff. This should be better.
Dammit. Visual Studio keeps running clang-format on me. I thought I had that turned off and fixed up the formatting but apparently not. I'll make sure those bits aren't in the next round.
Nov 10 2020
Thanks, Joel! It will likely take a few weeks for me to finish up the unittests part of my project, at which point this seems like a nice starting point for the FileCheck part. The basic infrastructure is definitely there.
Building Clang with Clang, adding the prototype patch improved build time by a barely perceptible 0.5%.
I prototyped a patch that would
- call flushLocalValueMap() at the top of selectInstruction()
- turn off the instruction-sinking stuff
- not erase the DbgLoc applied to local value instructions
Nov 9 2020
Nov 5 2020
I think we should also consider scrapping the local value map or maybe flushing it after selecting each instruction. I don't think we're getting a whole lot of value from sharing local values between multiple non-call instructions.
+ people who reviewed D43093.
Nov 4 2020
Oct 21 2020
Ummm the default expression-stack type is address-sized, and I'm not aware that we support machines with address sizes > 64 bits?
(DWARF 5 does have a typed stack, but IIUC this path is not used for DWARF 5.)
Sep 30 2020
Sep 29 2020
I was not aware that .eh_frame needed to be an ALLOC section, although it makes sense now that you say it; thanks for that!
I haven't touched the streamer side of AsmPrinter in a long time, but...
On PS4 we default to -fno-exceptions and while the section ends up named .eh_frame instead of .debug_frame the content is fine; we've had no complaints from our debugger folks. Aside from the introduction of the .cfi directives (which you do want), I'm not aware that there's any codegen consequence to this combination.
So, I'm not clear why a special streamer is really necessary.
Can we have a separate NFC refactor commit, to move that lump of code into its own emitCGProfile method? It might make the functional bits easier to follow. Also we have a private change in that area, and the ping-pong for this change has been a real headache.
Sep 23 2020
TL;DR: It's all good.
Sep 17 2020
Sep 14 2020
Sep 1 2020
Couple of nits and LGTM.
Aug 6 2020
Whoa--that's the *help* text? Well, if that's the only documentation for options that there is, I guess it has to go there; but it seems pretty excessive for the (ideally concise) output of clang --help.
Aug 4 2020
Looks okay (one grammar nit), probably worth waiting for someone else to chime in.
Jul 30 2020
No test?
Jul 28 2020
@rupprecht if you are still motivated to land this after so long, the only thing I see missing is to update the example from CHECK-SAME: 1{{$}} to CHECK-SAME: {{ 1$}} and then it LGTM. If not, there seems to be enough interest that someone else can land it?
Jul 23 2020
LGTM
Jul 20 2020
It looks like you have been tweaking a number of related SizeOf() methods. Is it practical to have just one? In a parent class, or even as a free function (as the size computation depends only on form/format, and is not related to the class at all).
I agree with @grimar as it looks like the change is from a DWARFDataExtractor without relocations to one with relocations.
Regarding header order, in cases like this it's usual to do an NFC commit to sort the headers that are there, and then the functional patch just adds the new one in the right place.
Jul 15 2020
Just wanted to say I agree this can be better than the Inputs/ idiom, especially for comparatively short inputs. We have some downstream tests that use a bunch of echo commands to generate a second file, but an extract utility would be cleaner IMO.
Jul 14 2020
According to https://bugs.llvm.org/show_bug.cgi?id=46653#c3 there is no problem on x86-64, suggesting the fix is more likely to be in the AArch64 target instead of in generic code. It's not uncommon for passes to lose source locations, or fail to add them when they should, so that is more likely to be the cause of the AArch64 issue.
Wouldn't using .ifdef directives with --defsym on the command line work equally well? At least for .s files run through llvm-mc.
There are examples of this in the lit tests already, easy enough to find.
Jul 9 2020
LGTM.