I just finished the work for PR21433. I think it's relatively low risk,
but it does change assembly output.
Should I commit this before or after the (imminent) 3.6 branch?
(I would have committed it tonight, but the buildbots are fairly red,
and I prefer to break out-of-tree code in the morning anyway.)
llvm.patch (372 KB)
clang.patch (20.2 KB)
Up to you at this point. If you’re confident that it’s solid and not going to break anything in-tree that’s already functioning go ahead. If you’re worried about the risk then you can wait - out of tree code isn’t a worry here.
-eric
One small request: Omitting the column field from MDLocation if it is 0 is fine, because it won’t make it into the line table. Omitting the line if it is 0 is not, because it gives the wrong impression that the line is being ignored, which is not the case. Line 0 will be emitted in the line table and has special semantics (Line 0 in DWARF may be used to mark compiler-generated code that has no corresponding source line number).
thanks,
adrian
One small request: Omitting the column field from MDLocation if it is 0 is
fine, because it won’t make it into the line table. Omitting the line if it
is 0 is not, because it gives the wrong impression that the line is being
ignored, which is not the case. Line 0 will be emitted in the line table
and has special semantics (Line 0 in DWARF may be used to mark
compiler-generated code that has no corresponding source line number).
I don't know - I think I might be OK with it being implicitly zero (I think
that's the column behavior - if the column is never set it's just zero in
the line table? (which is "no specific column") - that seems consistent
with zero line ("no actual line in the source code"))
The difference is that column 0 won’t be emitted into the line table, but line 0 will be — which is why I think it would less misleading to mirror this behavior in the IR.
– adrian
Original question is kind of moot at this point, looks like Hans has already created the branches.
Regarding line 0, as long as “has no associated line” doesn’t get mixed up with “inherits line from last previously set line” I’m okay.
–paulr
What about this:
- Still have `MDLocation(scope: !0)` imply `line: 0`.
- Change printout to include `line:` even when it's `0`.
There's a sane default, so we shouldn't have to specify it.
However, the value is important and relevant, so the assembly
writer *does* print it out.
Thoughts?
(Frankly I still prefer `line: 0` being omitted, but it did
seem kind of funny when I was updating tests that CHECK for
`line: 0` that they now check for `MDLocation(scope:`, so I
guess on balance I'm indifferent here.)
I'm pretty much in agreement with you on all counts there.
Omitting it is important for easy of writability of test cases.
I'm not sure it'll substantially hinder readability to have it omitted, so
I'm not in any hurry to have it print by default - but perhaps others feel
more strongly.
One small request: Omitting the column field from MDLocation if it is 0 is fine, because it won’t make it into the line table. Omitting the line if it is 0 is not, because it gives the wrong impression that the line is being ignored, which is not the case. Line 0 will be emitted in the line table and has special semantics (Line 0 in DWARF may be used to mark compiler-generated code that has no corresponding source line number).
I don't know - I think I might be OK with it being implicitly zero (I think that's the column behavior - if the column is never set it's just zero in the line table? (which is "no specific column") - that seems consistent with zero line ("no actual line in the source code”))
The difference is that column 0 won’t be emitted into the line table, but line 0 will be — which is why I think it would less misleading to mirror this behavior in the IR.
What about this:
- Still have `MDLocation(scope: !0)` imply `line: 0`.
- Change printout to include `line:` even when it's `0`.There's a sane default, so we shouldn't have to specify it.
However, the value is important and relevant, so the assembly
writer *does* print it out.
works for me!
-- adrian
One small request: Omitting the column field from MDLocation if it is 0 is fine, because it won’t make it into the line table. Omitting the line if it is 0 is not, because it gives the wrong impression that the line is being ignored, which is not the case. Line 0 will be emitted in the line table and has special semantics (Line 0 in DWARF may be used to mark compiler-generated code that has no corresponding source line number).
I don't know - I think I might be OK with it being implicitly zero (I think that's the column behavior - if the column is never set it's just zero in the line table? (which is "no specific column") - that seems consistent with zero line ("no actual line in the source code”))
The difference is that column 0 won’t be emitted into the line table, but line 0 will be — which is why I think it would less misleading to mirror this behavior in the IR.
What about this:
- Still have `MDLocation(scope: !0)` imply `line: 0`.
- Change printout to include `line:` even when it's `0`.There's a sane default, so we shouldn't have to specify it.
However, the value is important and relevant, so the assembly
writer *does* print it out.works for me!
r226046
Up to you at this point. If you're confident that it's solid and not going to break anything in-tree that's already functioning go ahead. If you're worried about the risk then you can wait - out of tree code isn't a worry here.
Nope, all the risky changes (i.e., splitting metadata/value) were before
the branch point ;).
I merged in the ones that missed the deadline (ending with r226095).