This is an archive of the discontinued LLVM Phabricator instance.

[MachineVerifier] Add a pass to ensure debug locations within a basic block are monotonically increasing
AbandonedPublic

Authored by davide on Apr 28 2020, 6:06 PM.

Diff Detail

Event Timeline

davide created this revision.Apr 28 2020, 6:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2020, 6:06 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
davide marked an inline comment as done.Apr 28 2020, 6:07 PM
davide added inline comments.
llvm/lib/CodeGen/MachineVerifier.cpp
329–330

Oops, this condition is inverted :(

davide updated this revision to Diff 260813.Apr 28 2020, 6:08 PM

Correct patch

paquette added inline comments.Apr 28 2020, 6:29 PM
llvm/lib/CodeGen/MachineVerifier.cpp
337–340

Would it make sense to use MI.isMetaInstruction() here instead? That should skip any instruction which does not generate code (including debug instructions.)

vsk requested changes to this revision.Apr 29 2020, 8:15 AM

I don’t think this should be part of MachineVerifier. The condition you’re checking for doesn’t indicate that the MIR is invalid. Even at -O0, you’ll hit it for code like:

int foo =

Call();

Where we (currently, at least) step to “Call()” before we step to the assignment. I doubt this is the only such edge case.

I do still think the condition would be useful to check, perhaps in a separate ‘checking’ pass for debug info.

This revision now requires changes to proceed.Apr 29 2020, 8:15 AM
In D79058#2010193, @vsk wrote:

I don’t think this should be part of MachineVerifier. The condition you’re checking for doesn’t indicate that the MIR is invalid. Even at -O0, you’ll hit it for code like:

int foo =

Call();

Where we (currently, at least) step to “Call()” before we step to the assignment. I doubt this is the only such edge case.

I do still think the condition would be useful to check, perhaps in a separate ‘checking’ pass for debug info.

In D79058#2010193, @vsk wrote:

I don’t think this should be part of MachineVerifier. The condition you’re checking for doesn’t indicate that the MIR is invalid. Even at -O0, you’ll hit it for code like:

int foo =

Call();

Where we (currently, at least) step to “Call()” before we step to the assignment. I doubt this is the only such edge case.

I do still think the condition would be useful to check, perhaps in a separate ‘checking’ pass for debug info.

I think this is a fair point, Vedant. Where would you put the verifier? Happy to chat about this further.

davide abandoned this revision.Apr 29 2020, 9:16 AM