Page MenuHomePhabricator

[TBLGEN] Fix subreg value overflow in DAGISelMatcher
ClosedPublic

Authored by rampitec on Feb 10 2020, 4:15 PM.

Details

Summary

Tablegen's DAGISelMatcher emits integers in a VBR format,
so if an integer is below 128 it can fit into a single
byte, otherwise high bit is set, next byte is used etc.
MatcherTable is essentially an unsigned char table. When
SelectionDAGISel parses the table it does a reverse translation.

In a situation when numeric value of an integer to emit is
unknown it can be emitted not as OPC_EmitInteger but as
OPC_EmitStringInteger using a symbolic name of the value.
In this situation the value should not exceed 127.

One of the situations when OPC_EmitStringInteger is used is
if we need to emit a subreg into a matcher table. However,
number of subregs can exceed 127. Currently last defined subreg
for AMDGPU is 192. That results in a silent bug in the ISel
with matcher reading from an invalid offset.

Fixed this bug to emit actual VBR encoded value for a subregs
which value exceeds 127.

Diff Detail

Event Timeline

rampitec created this revision.Feb 10 2020, 4:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 10 2020, 4:15 PM
Herald added subscribers: tpr, wdng. · View Herald Transcript
rampitec updated this revision to Diff 243703.Feb 10 2020, 5:03 PM

Moved check inside test .

arsenm added inline comments.Feb 10 2020, 5:45 PM
llvm/utils/TableGen/DAGISelMatcherGen.cpp
718

Spelling indicies

719

Typo cannout

722–725

I would except this to check a function in CodeGenRegBank rather than needing to do a find_if on a list

rampitec updated this revision to Diff 243747.Feb 10 2020, 11:19 PM
rampitec marked 3 inline comments as done.

Addressed review comments.

arsenm added inline comments.Feb 12 2020, 7:26 AM
llvm/test/TableGen/dag-isel-subregs.td
12

Can you split all of these register definitions into a template file in Common? I'll want to re-use this at some point

llvm/utils/TableGen/DAGISelMatcherGen.cpp
722

Actually, why do you need to do the name lookup at all? Can't you juts do CodeGenRegBank.getSubRegIdx(Def)?

rampitec updated this revision to Diff 244212.Feb 12 2020, 9:57 AM
rampitec marked 2 inline comments as done.

Addressed review comments.

rampitec marked an inline comment as done.Feb 12 2020, 10:56 AM
rampitec added inline comments.
llvm/utils/TableGen/DAGISelMatcherGen.cpp
722

I had to create another function which just lookups the map by Def, as CGP is constant. But you are right, string lookup was not needed.

arsenm accepted this revision.Feb 12 2020, 12:55 PM
This revision is now accepted and ready to land.Feb 12 2020, 12:55 PM
This revision was automatically updated to reflect the committed changes.
bcain added a subscriber: bcain.Feb 24 2020, 7:32 AM
bcain added inline comments.
llvm/utils/TableGen/RegisterInfoEmitter.cpp
176

Is it expected that the context for this .inc should have already included <cstdint> or that tablegen should be emitting that include in order to use this type?

rampitec marked an inline comment as done.Feb 24 2020, 10:12 AM
rampitec added inline comments.
llvm/utils/TableGen/RegisterInfoEmitter.cpp
176

That is produced by the tablegen in many different places, so I assume it should be already defined.