Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
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
18 changes: 18 additions & 0 deletions llvm/lib/CodeGen/MachineVerifier.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1274,6 +1274,24 @@ void MachineVerifier::verifyPreISelGenericInstruction(const MachineInstr *MI) {
if (TypeSize::isKnownGT(MMO.getSize().getValue(),
ValTy.getSizeInBytes()))
report("load memory size cannot exceed result size", MI);

if (MMO.getRanges()) {
auto operandBits = [](const MDOperand &o) -> unsigned {
ConstantInt *i = mdconst::dyn_extract<ConstantInt>(o);
if (!i->isNegative())
return i->getValue().getActiveBits();
APInt reversed(i->getValue());
Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't need to consider anything about the value of the range, only the type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is something I still do not understand:

The verifier should reject cases like this:
...
!0 = !{i64 -4294967295, i64 4294967296}
...
%3:_(<2 x s32>) = G_LOAD %0(p1) :: (volatile load (s64), align 4, !range !0, addrspace 1)

but if we get the information about the type, it is "i64" and there is a match between 2*32 == 64, no issues, you mentioned vector or scalar is not important, what would be the reason to reject this instruction?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@arsenm could you please help me understand this piece, it looks like if I rely on type information alone, the cases that are mentioned in the description of this bug would be still legal. Thank you!

Copy link
Contributor

Choose a reason for hiding this comment

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

there is a match between 2*32 == 64

This is the problem, it's not a match. The total bitwidth is not what matters, the scalar type does. It would be OK if this was a <2 x s64> result and the range was using i64. tttt

i.e. the vectorness doesn't matter, nor the total bit width. The scalar type interpretation on both halves is what matters

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you for the explanation, I will get back with updated code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated, please take a look

reversed.negate();
return reversed.getActiveBits() + 1; // one more bit for the sign
};
unsigned bitsUsed =
std::max(operandBits(MMO.getRanges()->getOperand(0)),
operandBits(MMO.getRanges()->getOperand(1)));
if (bitsUsed != ValTy.getScalarType().getSizeInBits()) {
report("range is incompatible with the value it gets assigned to",
MI);
}
}
} else if (MI->getOpcode() == TargetOpcode::G_STORE) {
if (TypeSize::isKnownLT(ValTy.getSizeInBytes(),
MMO.getSize().getValue()))
Expand Down
72 changes: 72 additions & 0 deletions llvm/test/MachineVerifier/test_g_load_vector_to_scalar.mir
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
# RUN: not --crash llc -mtriple=amdgcn-mesa-mesa3d -mcpu=tahiti -run-pass=none %s -o -
--- |
define void @range_metadata_sext_i8_signed_range_i64_load_as_v2i32() {
ret void
}

define void @range_metadata_sext_i8_signed_range_i64_load_as_v2i32_extractlo() {
ret void
}

define void @range_metadata_sext_i33_signed_range_i64_load_as_v2i32() {
ret void
}

!0 = !{i64 -4294967295, i64 4294967296}
!1 = !{i64 -8589934591, i64 8589934592}

...
---
name: range_metadata_sext_i33_signed_range_i64_load_as_v2i32
tracksRegLiveness: true
body: |
bb.0:
liveins: $vgpr0, $vgpr1
%1:_(s32) = COPY $vgpr0
%2:_(s32) = COPY $vgpr1
%0:_(p1) = G_MERGE_VALUES %1(s32), %2(s32)
; CHECK: Bad machine code: range is incompatible with the value it gets assigned to
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the verifier aborts after the first function fails, but keeps printing errors in the functions. As a consequence you would need all of these cases tested in one function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure if I understood it correctly, I put them all in one function, but now they look too similar, please take a look, thank you!

%3:_(<2 x s32>) = G_LOAD %0(p1) :: (volatile load (s64), align 4, !range !1, addrspace 1)
$vgpr0_vgpr1 = COPY %3
SI_RETURN implicit $vgpr0_vgpr1
...

---
name: range_metadata_sext_i8_signed_range_i64_load_as_v2i32_extractlo
tracksRegLiveness: true
body: |
bb.0:
liveins: $vgpr0, $vgpr1
%1:_(s32) = COPY $vgpr0
%2:_(s32) = COPY $vgpr1
%0:_(p1) = G_MERGE_VALUES %1(s32), %2(s32)
; CHECK: Bad machine code: range is incompatible with the value it gets assigned to
%3:_(<2 x s32>) = G_LOAD %0(p1) :: (volatile load (s64), align 4, !range !0, addrspace 1)
%zero:_(s32) = G_CONSTANT i32 0
%extract:_(s32) = G_EXTRACT_VECTOR_ELT %3, %zero
%6:_(s32) = G_SEXT_INREG %zero, 9
$vgpr0 = COPY %6
SI_RETURN implicit $vgpr0, implicit $vgpr1
...

---
name: range_metadata_sext_i8_signed_range_i64_load_as_v2i32
tracksRegLiveness: true
body: |
bb.0:
liveins: $vgpr0, $vgpr1
%1:_(s32) = COPY $vgpr0
%2:_(s32) = COPY $vgpr1
%0:_(p1) = G_MERGE_VALUES %1(s32), %2(s32)
; CHECK: Bad machine code: range is incompatible with the value it gets assigned to
%3:_(<2 x s32>) = G_LOAD %0(p1) :: (volatile load (s64), align 4, !range !0, addrspace 1)
%6:_(<2 x s32>) = G_SEXT_INREG %3, 9
$vgpr0_vgpr1 = COPY %6
SI_RETURN implicit $vgpr0_vgpr1
...
Loading