- User Since
- May 9 2013, 11:10 AM (413 w, 2 d)
Fri, Apr 9
Wed, Apr 7
Tue, Apr 6
Mon, Apr 5
Thu, Apr 1
FastISel is normally used only at -O0, I wouldn't expect any parameters to be optimized out at -O0.
The test is running llc with default optimization, which is -O2, and forcing fast-isel; this is not a usual combination and I wouldn't expect us to spend any effort making sure debug info works well with that combination.
I wasn't sure whether setUp() was supposed to be SetUp() or not. Good to have that confusion go away!
Tue, Mar 30
FTR, this looks okay from the PS4 owner's perspective, although IIRC @wristow did our original local patches.
Mon, Mar 29
Remove ifdefs, tweak descriptions/comments.
Fri, Mar 26
Problem has gone away, test assertions are now executed.
Thu, Mar 25
I think there are degrees of compatibility with regard to DWARF, which is inherently supposed to be extensible if the extension can be safely ignored by a consumer. This is the "permissive" rule in the standard.
Please add words to the effect of: When re-landing a reverted patch, the commit message should be updated to indicate the problem that was addressed (bot failure, cite the PR, whatever).
Wed, Mar 24
Welcome to the ranks!
Tue, Mar 23
Fri, Mar 19
+gribozavr who has been in this file most recently
Thu, Mar 18
Wed, Mar 17
Fri, Mar 12
Mar 8 2021
Mar 2 2021
As far as I know, it does. However, before CHECK-LABEL, a CHECK-NOT range is not the same as a CHECK range:
Mar 1 2021
The strange part to me is that a preceding directive's search range ends at the end of the CHECK-LABEL match. It seems like it ought to end at the start of the CHECK-LABEL match.
Feb 26 2021
Feb 25 2021
GNU ld has a rule "start_/stop_ references from a live input section retain the associated C identifier name sections",
which LLD may change in the future (D96914).
Feb 24 2021
Aha; attribute used *by itself* is not sufficient to preserve sections in the output. But the __start_/__stop_ symbols implicitly create a reference to each of the named sections, and that implicit reference can preserve them in the output (assuming gc roots etc).
So, the idea is that attribute retain can be used *instead* of the __start_/__stop_ symbols, to preserve sections in the output (with the advantage that it will work even for sections that do not have a C-identifier name).
Feb 23 2021
For ELF, used does not retain the entity, regardless of this patch.
A few typos. Nice to have these!
Feb 12 2021
Feb 10 2021
the driver had a redundant pass-through of the option
Feb 9 2021
I don't claim to be very familiar with this area but "round-trip" and "test" would make me think those bits should be in a lit test or unittest. As it is, it's not obvious what is functional and what is testing here.
Possibly I am misunderstanding something fundamental about how these options work?
Feb 8 2021
I've re-read a fair amount of the previous llvm-dev thread.
One goal of doing a patch like this was to collect some performance data; do we have that? In particular, performance data comparing link times with the option default-on versus default-off, so we can determine whether the difference is small enough that we should just do this processing unconditionally.
Feb 5 2021
Modified tests as recommended by Quentin on llvm-dev; I hope I got it right. Wasn't clear if the intent was to modify *all* tests in the file the same way (adding B.setInstr(foo)).
Feb 4 2021
Seems reasonable, provided you correct the language to be about building a cross-compiler rather than actually cross-compiling.
Jan 29 2021
I asked about this on llvm-dev but it seemed worth reporting here as well.
Some of the differences between the expected output (per the FileCheck directives) and the actual output appear to be little more than reordering of the instructions. However, oddly, in some cases the use of a value might precede its definition. For example, in NarrowSEXTINREG, the test is looking for this:
CHECK: [[T0:%[0-9]+]]:_(s16) = G_TRUNC CHECK: [[T1:%[0-9]+]]:_(s10) = G_TRUNC [[T0]]:_s(16) CHECK: [[T2:%[0-9]+]]:_(s10) = G_SEXT_INREG [[T1]]:_, 8 CHECK: [[T3:%[0-9]+]]:_(s16) = G_SEXT [[T2]]:_(s10)
But, what I see is this:
%5:_(s16) = G_SEXT %7:_(s10) %4:_(s16) = G_TRUNC %0:_(s64) %7:_(s10) = G_SEXT_INREG %6:_, 8 %6:_(s10) = G_TRUNC %4:_(s16)
So the SEXT uses the result of SEXT_INREG, which uses the result of the second TRUNC; these are all forward references. That doesn't seem correct; uses before defs? If there's a problem with instruction ordering, fixing that seems like its own issue, and would probably make many of the failures I'm seeing just go away. (Global ISel is very much not my area, so I'd appreciate it if someone else could figure out the ordering question.)
Jan 25 2021
Jan 22 2021
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.
Jan 21 2021
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.
Jan 19 2021
Catching up on this, sorry...
Jan 11 2021
Jan 6 2021
Jan 5 2021
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
See D92606 for a front-end patch to improve locations in the IR.
That, plus reapplying this patch, should help out GDB. I haven't had a chance to run the suite myself with both patches applied and I'm basically off tomorrow, so if @dblaikie doesn't have a chance, I'll do that on Monday.