Page MenuHomePhabricator

[GlobalISel] Assign the correct debug location when combining G_SEXT/G_ANYEXT/G_ZEXT
ClosedPublic

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

Diff Detail

Event Timeline

davide created this revision.Apr 28 2020, 12:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2020, 12:06 PM
Herald added a subscriber: rovka. · View Herald Transcript

The code looks fine to me, but I think the testcase can be tidied up a bit.

llvm/test/CodeGen/AArch64/GlobalISel/combine-ext-debugloc.mir
2

Should this have -verify-machineinstrs?

9–10

Probably can just remove this?

38–39

Would be nice to replace the paths with dummy paths?

43

Would be nice to replace this path with a dummy path?

52–59

I think you can remove most of these?

60–74

I think you can remove this too and the test should still work.

76–94

I think you can remove all of this?

davide marked an inline comment as done.Apr 28 2020, 1:49 PM
davide added inline comments.
llvm/test/CodeGen/AArch64/GlobalISel/combine-ext-debugloc.mir
9–10

Just afraid it might freak out when run on linux, but I guess passing -mtriple explicitly overrides the setting?

vsk added a comment.Apr 28 2020, 1:50 PM

I'm not sure the G_SEXT case is covered -- could you add a sext(trunc x) pattern to the MIR?

Also, I think you can delete the IR contents of @main, those shouldn't be needed to construct the MIR.

aemerson accepted this revision.Apr 28 2020, 1:51 PM
This revision is now accepted and ready to land.Apr 28 2020, 1:51 PM
vsk added inline comments.Apr 28 2020, 1:52 PM
llvm/test/CodeGen/AArch64/GlobalISel/combine-ext-debugloc.mir
9–10

IME the target data layout & triple sometimes do matter, as some builders may be configured with different assumptions about what these are by default.

davide updated this revision to Diff 260759.Apr 28 2020, 1:55 PM

Addressed Jessica's feedback.

In D79031#2008718, @vsk wrote:

I'm not sure the G_SEXT case is covered -- could you add a sext(trunc x) pattern to the MIR?

Good point, I'll revert that bit & do that in a subsequent commit.

Also, I think you can delete the IR contents of @main, those shouldn't be needed to construct the MIR.

That didn't work.

dsanders accepted this revision.Apr 28 2020, 2:10 PM

LGTM but double check -verify-machineinstrs works with -run-pass

llvm/test/CodeGen/AArch64/GlobalISel/combine-ext-debugloc.mir
2

Also, does it actually run? It used to be the case that -start-before=X -stop-after=X ran it but -run-pass=X didn't. I don't remember off-hand whether that got fixed

40–50

The 'registers' section can be safely removed

davide closed this revision.Apr 28 2020, 2:12 PM

I guess I addressed everybody's comments.

commit 0ed276bb08a476a4d44d96afad7c077876a8c0ed (HEAD -> master, origin/master, origin/HEAD)
Author: Davide Italiano <ditaliano@apple.com>
Date: Tue Apr 28 13:09:28 2020 -0700

[GlobalISel] Assign the correct debug location when combining G_ANYEXT/G_ZEXT

<rdar://problem/62535712>
davide marked an inline comment as done.Apr 28 2020, 2:13 PM
davide added inline comments.
llvm/test/CodeGen/AArch64/GlobalISel/combine-ext-debugloc.mir
40–50

The test fails if I do.