This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Fix failing assert with scratch ST mode
ClosedPublic

Authored by sebastian-ne on Jan 11 2021, 5:30 AM.

Details

Summary

In ST mode, flat scratch instructions have neither an sgpr nor a vgpr
for the address. This lead to an assertion when inserting hard clauses.

Diff Detail

Event Timeline

sebastian-ne created this revision.Jan 11 2021, 5:30 AM
sebastian-ne requested review of this revision.Jan 11 2021, 5:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 11 2021, 5:30 AM
foad added a comment.Jan 11 2021, 5:44 AM

I think the intention was that getMemOperandsWithOffsetWidth should return false if no base operands were found (perhaps MachineScheduler could assert this?): https://reviews.llvm.org/source/llvm-github/browse/main/llvm/include/llvm/CodeGen/TargetInstrInfo.h$1304

This comment is out of date anyway: https://reviews.llvm.org/source/llvm-github/browse/main/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp$390

I think the intention was that getMemOperandsWithOffsetWidth should return false if no base operands were found (perhaps MachineScheduler could assert this?)

I’d argue that in this case, we found the base operands and we know that except for the constant offset, they are zero.
Also, we want these operations to be claused, I think this is the likely case for stack accesses in entry-points.

Should we adjust the documentation comment to something like this?

It returns false if no base operands and offset was found.

→ It returns false if no base operands and offset could be determined.

foad added a comment.Jan 11 2021, 6:21 AM

I think the intention was that getMemOperandsWithOffsetWidth should return false if no base operands were found (perhaps MachineScheduler could assert this?)

I’d argue that in this case, we found the base operands and we know that except for the constant offset, they are zero.
Also, we want these operations to be claused, I think this is the likely case for stack accesses in entry-points.

OK, sounds reasonable.

Should we adjust the documentation comment to something like this?

It returns false if no base operands and offset was found.

→ It returns false if no base operands and offset could be determined.

That's a very subtle change! I think it would be better to be more explicit, e.g.:

/// Get zero or more base operands and the byte offset of an instruction that reads/writes
/// memory. Note that there may be zero base operands if the instruction accesses a constant address.

(Second sentence optional depending on how verbose you're feeling.)

Your formulation is definitely better.

From what I see, that change should be fine with other targets.
PowerPC asserts that BaseOps.size() == 1, which is fine as there is always exactly one operand if getMemOperandsWithOffsetWidth returns true there.

Add test case for clausing two flat scratch instructions with constant adresses.

foad accepted this revision.Jan 11 2021, 7:13 AM

LGTM with comment fix.

llvm/include/llvm/CodeGen/TargetInstrInfo.h
1305

Remove "no".

This revision is now accepted and ready to land.Jan 11 2021, 7:13 AM
rampitec accepted this revision.Jan 11 2021, 10:30 AM
This revision was automatically updated to reflect the committed changes.