This is an archive of the discontinued LLVM Phabricator instance.

[MachineInstr] In addRegisterKilled and addRegisterDead, don't remove operands from inline assembly instructions if they have an associated flag operand.
ClosedPublic

Authored by craig.topper on Sep 7 2018, 5:46 PM.

Details

Summary

INLINEASM instructions use extra operands to carry flags. If a register operand is removed without removing the flag operand, then the flags will no longer make sense.

This patch tries to fix this by preventing the removal when a flag operand is present. I have no idea if this is the right thing to do here.

This fixes a crash for the following test. Previously the Live Variable analysis would remove the edx and eax clobber operands since they are redundant with the rax and rdx operands. But it did not remove their flag operands. A later pass expected a register operand to follow the flag operand, but it was no longer there.

This code was created with MS inline assembly which added clobbers for each instruction without knowing about sub/super registers.

target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

@test_mem = dso_local global [16 x i8] c"UUUUUUUUUUUUUUUU", align 16

; Function Attrs: nounwind uwtable
define dso_local void @foo() local_unnamed_addr {
entry:
  tail call void asm sideeffect inteldialect "clc\0A\09cmpxchg8b $0\0A\09cmpxchg16b $1\0A\09clc", "=*m,=*m,~{eax},~{edx},~{flags},~{rax},~{rdx},~{dirflag},~{fpsr},~{flags}"([16 x i8]* nonnull @test_mem, [16 x i8]* nonnull @test_mem) #1
  ret void
}

Diff Detail

Event Timeline

craig.topper created this revision.Sep 7 2018, 5:46 PM
rnk added a comment.Sep 10 2018, 4:45 PM

Sounds good, it could use an IR test though.

BTW, if you want to clean up these clobber lists for MS inline asm, they are computed in llvm/lib/MC/MCParser/AsmParser.cpp. I think that has access to MCRegisterInfo, which knows the super/sub register relationships. This fix on its own seems like a good idea, though.

craig.topper edited the summary of this revision. (Show Details)Sep 10 2018, 9:46 PM

Add the test case from the description.

rnk requested changes to this revision.Sep 13 2018, 1:13 PM

lgtm

This revision now requires changes to proceed.Sep 13 2018, 1:13 PM
craig.topper accepted this revision.Sep 13 2018, 1:44 PM

I'm going to assume Reid meant to hit approve instead of changes required.

This revision was not accepted when it landed; it landed in state Needs Revision.Sep 13 2018, 1:52 PM
This revision was automatically updated to reflect the committed changes.
rnk added a comment.Sep 13 2018, 2:24 PM

Right, sorry about that. :)

malharJ added a subscriber: malharJ.EditedJun 17 2022, 2:57 AM

The added condition

findInlineAsmFlagIdx(OpIdx) < 0))

avoids removing the Machine operand altogether (if there exists a corresponding inline asm flag),
was there any reason why the inline asm flag was not updated instead ?
ie. the code could still remove the machine operand but update (or remove it completely?) the asm flag to reflect that there is now one less operand ?

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 17 2022, 2:57 AM

I assume the reason is just that it would be complicated to rewrite the asm flags.