This is an archive of the discontinued LLVM Phabricator instance.

[CodeGenPrepare] Also consider metadata uses
Needs RevisionPublic

Authored by loladiro on Jan 16 2016, 3:07 PM.

Details

Summary

When promoting extensions to wider types, CodeGenPrepare checks whether there is only the one use, and if so does not insert a corresponding trunc instruction to compensate for the extension. However, in doing so it does not consider any uses in debug info. This is undesirable, because that way the wrong size will be passed to a dbg.* intrinsic, which is incorrect IR (and I'm in the process of adding a Verifier check for).

Diff Detail

Repository
rL LLVM

Event Timeline

loladiro updated this revision to Diff 45084.Jan 16 2016, 3:07 PM
loladiro retitled this revision from to [CodeGenPrepare] Also consider metadata uses.
loladiro updated this object.
loladiro added reviewers: aprantl, qcolombet.
loladiro set the repository for this revision to rL LLVM.
loladiro added a subscriber: llvm-commits.
hfinkel requested changes to this revision.Jan 16 2016, 8:41 PM
hfinkel added a reviewer: hfinkel.
hfinkel added a subscriber: hfinkel.

Unfortunately, you cannot solve the problem this way. The largest reason is that it would cause the presence or absence of debug info to affect code generation, which it should not do - it becomes very difficult to debug problems when adding -g makes the problem go away ;) -- And secondly, in a conflict between performing an optimization and preserving debug info, we always perform the optimization, even if that means we need to discard the debug info. Is there no way to update the debug info instead?

This revision now requires changes to proceed.Jan 16 2016, 8:41 PM

Yeah, I was fearing something like that, although I had hoped that since this won't actually show up in the generated assembly (or at least it shouldn't), it might be ok. In any case, I don't think this is representable in debug info currently without having an actual trunc instruction. We could modify the DIExpression to be able to express truncation, but that's somewhat tricky, since DWARF does not have such an operation. @aprantl, thoughts?

Yeah, I was fearing something like that, although I had hoped that since this won't actually show up in the generated assembly (or at least it shouldn't), it might be ok.

In general, anything you do to change the IR can change the code generated. If nothing else, there are a lot of transformations and analysis in the backend that, say, only look back through N instructions (for some fixed N).

In any case, I don't think this is representable in debug info currently without having an actual trunc instruction.

We could modify the DIExpression to be able to express truncation, but that's somewhat tricky, since DWARF does not have such an operation. @aprantl, thoughts?

This is certainly not my area of expertise, but DWARF does have the ability to represent data in only parts of registers, so it seems like there should be some way to express what you want. If the values are sign/zeroextended, is there an actual problem with the DWARF being generated (i.e. would garbage somehow appear in the debugger), or is it just the upcoming verifier that would complain?

As far as I can tell, DWARF values are implicitly truncated. The problem is not the final DWARF representation but our IR representation. Passes like SROA rely on the sizing information of IR and debug information to match, which is what the upcoming Verifier stuff is supposed to ensure. Of course, SROA will never run after CodeGenPrepare, so it's not an actual problem, but it would be very annoying to not be able to have the Verifier check this invariant, because of this one thing. I do think it might be reasonable to have an (OP_trunc size) for the DIExpression. It could get ignored by the backend, but would allow us to do proper verification.

As far as I can tell, DWARF values are implicitly truncated. The problem is not the final DWARF representation but our IR representation. Passes like SROA rely on the sizing information of IR and debug information to match

Why does it make this assumption? Do other passes that widen variables (e.g. indvarsimplify) also trigger this problem?

, which is what the upcoming Verifier stuff is supposed to ensure. Of course, SROA will never run after CodeGenPrepare, so it's not an actual problem, but it would be very annoying to not be able to have the Verifier check this invariant, because of this one thing. I do think it might be reasonable to have an (OP_trunc size) for the DIExpression. It could get ignored by the backend, but would allow us to do proper verification.

Say you're splitting an Alloca of a 64bit thing into 2 32bit things, SROA, will rewrite the debug info to say, ok, the lower 32bits are here, the upper 32bits are there. Now, if the variable is only declared to be 32bits in debug info, the verifier and backend will barf and say, "hey the piece you're describing is outside of the range of this variable". I had a quick look at indvarsimplify, but as far as I can tell it inserts compensating truncs in all cases, so does not suffer from this problem (but I only had a cursory look, somebody else might have to answer whether it ever RAUWs a value with a value of a different size).

Say you're splitting an Alloca of a 64bit thing into 2 32bit things, SROA, will rewrite the debug info to say, ok, the lower 32bits are here, the upper 32bits are there. Now, if the variable is only declared to be 32bits in debug info, the verifier and backend will barf and say, "hey the piece you're describing is outside of the range of this variable".

Maybe we should just drop those pieces instead of asserting?

I had a quick look at indvarsimplify, but as far as I can tell it inserts compensating truncs in all cases, so does not suffer from this problem (but I only had a cursory look, somebody else might have to answer whether it ever RAUWs a value with a value of a different size).

It inserts truncs when there are other users it can't also widen; but it does generate new instructions instead of mutating the type of an existing instruction.

However, based on what you're telling me, this is worse (in a sense), because it implies that we're losing debug info on the induction variable when we do this. If we were to update indvarsimplify to attempt to preserve more debug info, we'd run into the same problem?

Say you're splitting an Alloca of a 64bit thing into 2 32bit things, SROA, will rewrite the debug info to say, ok, the lower 32bits are here, the upper 32bits are there. Now, if the variable is only declared to be 32bits in debug info, the verifier and backend will barf and say, "hey the piece you're describing is outside of the range of this variable".

Maybe we should just drop those pieces instead of asserting?

Perhaps, but I don't think that's a particularly sounds design, because that also means we drop cases where there are legitimate bugs in transformations or the frontend.

I had a quick look at indvarsimplify, but as far as I can tell it inserts compensating truncs in all cases, so does not suffer from this problem (but I only had a cursory look, somebody else might have to answer whether it ever RAUWs a value with a value of a different size).

It inserts truncs when there are other users it can't also widen; but it does generate new instructions instead of mutating the type of an existing instruction.

However, based on what you're telling me, this is worse (in a sense), because it implies that we're losing debug info on the induction variable when we do this. If we were to update indvarsimplify to attempt to preserve more debug info, we'd run into the same problem?

Not sure I understand what you're saying here (probably just because I'm not familiar with the pass). The problem here occurs if you RAUW an instruction of some type by an instruction of a larger (or smaller type).

Say you're splitting an Alloca of a 64bit thing into 2 32bit things, SROA, will rewrite the debug info to say, ok, the lower 32bits are here, the upper 32bits are there. Now, if the variable is only declared to be 32bits in debug info, the verifier and backend will barf and say, "hey the piece you're describing is outside of the range of this variable".

Maybe we should just drop those pieces instead of asserting?

Perhaps, but I don't think that's a particularly sounds design, because that also means we drop cases where there are legitimate bugs in transformations or the frontend.

I'm obviously not against you pursuing a design that allows us to explicitly capture widening and then verify accordingly. However, if that is not immediately possible for whatever reason, loosening the constraint is also a potential solution in the mean time.

I had a quick look at indvarsimplify, but as far as I can tell it inserts compensating truncs in all cases, so does not suffer from this problem (but I only had a cursory look, somebody else might have to answer whether it ever RAUWs a value with a value of a different size).

It inserts truncs when there are other users it can't also widen; but it does generate new instructions instead of mutating the type of an existing instruction.

However, based on what you're telling me, this is worse (in a sense), because it implies that we're losing debug info on the induction variable when we do this. If we were to update indvarsimplify to attempt to preserve more debug info, we'd run into the same problem?

Not sure I understand what you're saying here (probably just because I'm not familiar with the pass). The problem here occurs if you RAUW an instruction of some type by an instruction of a larger (or smaller type).

I understand; if we widen by direct type mutation (i.e. RAUW an instruction by an another of a different type while fixing everything else up) then we trigger this inconsistency with the debug information. If we widen by creating new variables to replace the old, and indvarsimplify does, then we essentially drop the debug info. Dropping the debug info does not trigger the problem, obviously, but it also means we've dropped the debug info (which is not really "better"). If we added code to indvarsimplify to attempt to preserve debug info (transfer the debug info associated with the old induction variables to the new ones) then, as I understand it, we'd end up triggering the same problem as you're seeing with CGP.