This is an archive of the discontinued LLVM Phabricator instance.

[TableGen] Prevent invalid code generation when emitting AssemblerPredicate conditions.
ClosedPublic

Authored by tomatabacu on Mar 12 2015, 9:10 AM.

Details

Summary

The loop which emits AssemblerPredicate conditions also links them together by emitting a '&&'.
If the 1st predicate is not an AssemblerPredicate, while the 2nd one is, nothing gets emitted for the 1st one, but we still emit the '&&' because of the 2nd predicate.
This generated code looks like "( && Cond2)" and is invalid.

Diff Detail

Event Timeline

tomatabacu updated this revision to Diff 21839.Mar 12 2015, 9:10 AM
tomatabacu retitled this revision from to [TableGen] Do not link Predicate conditions if the 1st one wasn't emitted..
tomatabacu updated this object.
tomatabacu edited the test plan for this revision. (Show Details)
tomatabacu added a subscriber: Unknown Object (MLST).

I don't think the commit message explains this change very well. If we hadn't already discussed the problem off-list I'm not sure I would have understood what you meant. I'd have started with a subject like:

[tablegen] Fix invalid code generation when the first predicate is not an AssemblerMatcherPredicate.

and then talked about the way it currently produces 'if ( && B)' in the body of the message.

test/TableGen/PredicateOrderDisasm.td
3–5
3–11

This is a bit strange to read. Maybe simply:

Check that we don't accidentally generate code of the form 'if ( && B)'.

is enough.

37

It's better to check that the correct thing happens rather than check that the wrong thing doesn't happen.

tomatabacu updated this revision to Diff 22086.Mar 17 2015, 4:46 AM
tomatabacu retitled this revision from [TableGen] Do not link Predicate conditions if the 1st one wasn't emitted. to [TableGen] Prevent invalid code generation when emitting AssemblerPredicate conditions..
tomatabacu updated this object.

Rewrote comments.
Switched to CHECK-ing for correct behavior.
Renamed test file to a more appropriate name.

dsanders accepted this revision.Mar 30 2015, 9:12 AM
dsanders added a reviewer: dsanders.

LGTM

This revision is now accepted and ready to land.Mar 30 2015, 9:12 AM
tomatabacu closed this revision.Apr 7 2015, 5:13 AM