Skip to content

[TargetLowering] Use Correct VT for Multi-out Asm #116024

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 14, 2024
Merged

[TargetLowering] Use Correct VT for Multi-out Asm #116024

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7759232
[TargetLowering] Use Correct VT for Multi-out Asm
lenary Nov 13, 2024
3b0198c
fixup! [TargetLowering] Use Correct VT for Multi-out Asm
lenary Nov 13, 2024
c5f8190
fixup! fixup! [TargetLowering] Use Correct VT for Multi-out Asm
lenary Nov 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5753,7 +5753,8 @@ TargetLowering::ParseConstraints(const DataLayout &DL,
assert(!Call.getType()->isVoidTy() && "Bad inline asm!");
if (auto *STy = dyn_cast<StructType>(Call.getType())) {
OpInfo.ConstraintVT =
getSimpleValueType(DL, STy->getElementType(ResNo));
getAsmOperandValueType(DL, STy->getElementType(ResNo))
.getSimpleVT();
Comment on lines +5756 to +5757
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens for nested structs? Are those supported?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe the lowering from an inline assembly statement to a call ... asm will create a nested struct. It either returns the single type corresponding to the output (if there's one output), or a struct with a member per output (if there's multiple outputs). I think that inline asm inputs/outputs cannot themselves be structs, but I'm not 100% certain of that. This code would suggest not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tried it and it seems to hit unreachable "Unknown type!"; we should do better here and report a proper error

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better errors for IR that frontends don't produce seems like a different issue to fixing an actual crash in the AArch64 backend that frontends can hit, so should be in a different PR IMO.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, separate fix

} else {
assert(ResNo == 0 && "Asm only has one result!");
OpInfo.ConstraintVT =
Expand Down
36 changes: 34 additions & 2 deletions llvm/test/CodeGen/AArch64/ls64-inline-asm.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
; RUN: llc -mtriple=aarch64 -mattr=+ls64 -verify-machineinstrs -o - %s | FileCheck %s

%struct.foo = type { [8 x i64] }

define void @load(ptr %output, ptr %addr) {
; CHECK-LABEL: load:
; CHECK: // %bb.0: // %entry
Expand Down Expand Up @@ -103,3 +101,37 @@ entry:
call void asm sideeffect "st64b $0,[$1]", "r,r,~{memory}"(i512 %s.sroa.0.0.insert.insert, ptr %addr)
ret void
}

define void @multi_output(ptr %addr) {
; CHECK-LABEL: multi_output:
; CHECK: // %bb.0: // %entry
; CHECK-NEXT: //APP
; CHECK-NEXT: ld64b x0, [x0]
; CHECK-NEXT: mov x8, x0
; CHECK-NEXT: //NO_APP
; CHECK-NEXT: stp x6, x7, [x8, #48]
; CHECK-NEXT: stp x4, x5, [x8, #32]
; CHECK-NEXT: stp x2, x3, [x8, #16]
; CHECK-NEXT: stp x0, x1, [x8]
; CHECK-NEXT: ret
entry:
%val = call { i512, ptr } asm sideeffect "ld64b $0, [$2]; mov $1, $2", "=r,=r,r,~{memory}"(ptr %addr)
%val0 = extractvalue { i512, ptr } %val, 0
%val1 = extractvalue { i512, ptr } %val, 1
store i512 %val0, ptr %val1, align 8
ret void
}

; FIXME: This case still crashes in RegsForValue::AddInlineAsmOperands without
; additional changes. I believe this is a bug in target-independent code, that
; is worked around in the RISC-V and SystemZ backends, but should almost
; certainly be fixed instead.
; define void @tied_constraints(ptr %addr) {
; entry:
; %in = load i512, ptr %addr, align 8
; %val = call { i512, ptr } asm sideeffect "nop", "=r,=r,0,1,~{memory}"(i512 %in, ptr %addr)
; %val0 = extractvalue { i512, ptr } %val, 0
; %val1 = extractvalue { i512, ptr } %val, 1
; store i512 %val0, ptr %val1, align 8
; ret void
; }
Loading