This is an archive of the discontinued LLVM Phabricator instance.

[AsmPrinter] Loop over bundles in generic code
AbandonedPublic

Authored by sebastian-ne on Sep 20 2021, 2:26 AM.

Details

Summary

Every target, except Hexagon, iterates over bundles and prints the
contents like every other instruction.

A part of instruction printing is done in the for-loop in the
AsmPrinter, e.g. handling debug or kill instructions, which had to be
reimplemented in every target to support bundles.

With this patch, iterating over instruction bundles is taken out of the
targets and moved into the generic AsmPrinter.
For Hexagon, the bundles are still printed when encountering the bundle
instruction. Further instructions inside bundles are ignored.

Mips has a test change for some debug data. I hope this is just some
moved metadata and doesn't change behavior.

Diff Detail

Event Timeline

sebastian-ne created this revision.Sep 20 2021, 2:26 AM
sebastian-ne requested review of this revision.Sep 20 2021, 2:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 20 2021, 2:26 AM
foad added a comment.Sep 20 2021, 2:38 AM

I guess if you re-auto-generated test checks, there would be millions of changes due to extra comments at the end of lines? But they don't show up as failures because the generated checks don't end with {{$}}?

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1275–1290

Can't you change this loop to iterate over MBB.instrs(), instead of adding another nested loop?

sebastian-ne marked an inline comment as done.

Good idea to use .instrs(), it looks better now.

I guess if you re-auto-generated test checks, there would be millions of changes due to extra comments at the end of lines? But they don't show up as failures because the generated checks don't end with {{$}}?

Right, they don’t show up as failures. I hope it wouldn’t be millions, the comment is only new for spills inside bundles/clauses. I can search for such cases and regenerate the tests if you think it makes sense?

foad added a comment.Sep 20 2021, 3:00 AM

I can search for such cases and regenerate the tests if you think it makes sense?

I think it would be nice, to avoid spurious changes the next time someone has to regenerate the checks, but I won't insist.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1275

Much nicer! :)

foad added a comment.Sep 24 2021, 7:23 AM

Mips has a test change for some debug data. I hope this is just some
moved metadata and doesn't change behavior.

Unfortunately I think you have broken the delay slot stuff that was added in D78107, which was meant to ensure that the "label after a call follows the delay slot instruction". The diff from your change on the mips test case looks like this:

 	jal	f1
-	addiu	$4, $zero, 10
 $tmp2:
+	addiu	$4, $zero, 10

so the label $tmp2 now points to the addiu which is in the delay slot of the jal (call).

It looks like D78107 relies on some dubious bundle-without-a-BUNDLE-insn constrructs like this:

JAL @f1, <regmask $fp $ra $d10 $d11 $d12 $d13 $d14 $d15 $f20 $f21 $f22 $f23 $f24 $f25 $f26 $f27 $f28 $f29 $f30 $f31 $s0 $s1 $s2 $s3 $s4 $s5 $s6 $s7>, implicit-def dead $ra, implicit killed $a0, implicit-def $sp, debug-location !17; m.c:0 {
  $a0 = ADDiu $zero, 10, debug-location !17; m.c:0
}

so maybe you just need to be careful to handle these correctly?

I think letting generic code inspect individual instructions inside bundles violates the spirit of what bundles, which should generally be opaque to generic code. I wouldn't be surprised if this breaks some out of tree target using bundles

llvm/lib/Target/Hexagon/HexagonAsmPrinter.cpp
761

No else after return

I think letting generic code inspect individual instructions inside bundles violates the spirit of what bundles, which should generally be opaque to generic code. I wouldn't be surprised if this breaks some out of tree target using bundles

In that case, I’ll abandon this.

sebastian-ne abandoned this revision.Sep 27 2021, 1:54 AM
foad added a comment.Sep 27 2021, 2:06 AM

I think letting generic code inspect individual instructions inside bundles violates the spirit of what bundles, which should generally be opaque to generic code. I wouldn't be surprised if this breaks some out of tree target using bundles

Well it seems OK to me, given that it simplifies all the in-tree emitInstruction implementations.

Maybe the API of emitInstruction could change to take a reference to an iterator, and update the iterator to indicate how many instructions it has emitted? That way all the in-tree implementations could emit a single MachineInstr but your hypothetical out-of-tree implementation could emit an entire bundle at once.