This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Check if register is non-null before calling isSubRegisterEq (NFCI)
ClosedPublic

Authored by barannikov88 on May 23 2023, 10:11 PM.

Details

Summary

D151036 adds an assertions that prohibits iterating over sub- and
super-registers of a null register. This is already the case when
iterating over register units of a null register, and worked by
accident for sub- and super-registers.

Diff Detail

Event Timeline

barannikov88 created this revision.May 23 2023, 10:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 23 2023, 10:11 PM
barannikov88 requested review of this revision.May 23 2023, 10:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 23 2023, 10:11 PM
foad accepted this revision.May 23 2023, 11:35 PM

D151036 adds an assertions that prohibits iterating over sub- and
super-registers of a null register. This is already the case when
iterating over register units of a null register, and worked by
accident for sub- and super-registers.

OK. I had been wondering if it made sense to treat 0 as a kind of empty register, i.e. no regunits, no subregs, no aliases, (not sure about superregs). But if the existing code did not allow you to iterate its regunits then I think it's pretty clear that this was not the intended design.

llvm/lib/Target/AMDGPU/SIFrameLowering.cpp
568

@arsenm will ask you to "De Morgan" this :)

This revision is now accepted and ready to land.May 23 2023, 11:35 PM

Apply the suggestion and rebase on top of 8e16365c to get a green mark

foad accepted this revision.May 24 2023, 12:00 AM
barannikov88 marked an inline comment as done.EditedMay 24 2023, 12:11 AM

D151036 adds an assertions that prohibits iterating over sub- and
super-registers of a null register. This is already the case when
iterating over register units of a null register, and worked by
accident for sub- and super-registers.

OK. I had been wondering if it made sense to treat 0 as a kind of empty register, i.e. no regunits, no subregs, no aliases, (not sure about superregs). But if the existing code did not allow you to iterate its regunits then I think it's pretty clear that this was not the intended design.

I found only a few cases where this caused problems so far (see the stack). There may be more hidden, but overall I think this change is in the right direction.
I wish there was a way to enumerate all registers without a for-loop like for (i = 0( or 1?), e = TRI->getNumRegs(); i != e; ++i), this would halven the number of issues.