Page MenuHomePhabricator

[globalisel][tablegen] Simplify named operand/operator lookups and fix a wrong-code bug this revealed.

Authored by dsanders on Aug 11 2017, 7:49 AM.



Operand variable lookups are now performed by the RuleMatcher rather than
searching the whole matcher hierarchy for a match. This revealed a wrong-code
bug that currently affects ARM and X86 where patterns that use a variable more
than once in the match pattern will be imported but won't check that the
operands are identical. This can cause the tablegen-erated matcher to
accept matches that should be rejected.

Depends on D36569

Diff Detail


Event Timeline

dsanders created this revision.Aug 11 2017, 7:49 AM
dsanders retitled this revision from [globalisel][tablegen] Simplify and fix a wrong-code bug this revealed. to [globalisel][tablegen] Simplify named operand/operator lookups and fix a wrong-code bug this revealed..Aug 11 2017, 7:59 AM
dsanders updated this revision to Diff 118446.Oct 10 2017, 11:50 AM

Rebase and ping

qcolombet edited edge metadata.Oct 12 2017, 11:39 AM

Looks mostly good.
Couple of comments below.

158 ↗(On Diff #118446)

Food for thought: I wonder if we should use a different name like CheckIsSame, because when I see tie, it makes me think of the tie operand in the MCDesc and that's not what this is.
I believe I won't be the only one confused by this at first :).

15 ↗(On Diff #118446)

Could you add a comment explaining what this test is checking, beyond blsr selection?
In particular, I would have expected something about the fact that the operands of the both G_ADD and G_AND should be the same.

Could you also add a negative test case?
In particular, one that would have match with the bug but does now. I expect you could just change the parameters of one of G_ADD or G_ADD.

2091 ↗(On Diff #118446)

This is an optimization, not a correctness problem, right?

2092 ↗(On Diff #118446)

Could we have this check pushed into ::addPredicate?
That way we wouldn't have to spread it all around.

dsanders added inline comments.Oct 12 2017, 1:47 PM
158 ↗(On Diff #118446)

I see your point, I'll rename it. The reason I went with Tie was that the use of the opcode is triggered by using the same name multiple times like the MC layer.

15 ↗(On Diff #118446)


2091 ↗(On Diff #118446)

That's right. It's not wrong to check the original predicate again, it just saves on work.

2092 ↗(On Diff #118446)

I think so, I'll give it a go

dsanders updated this revision to Diff 118989.Oct 13 2017, 4:32 PM
dsanders marked 6 inline comments as done.

Fix nits aside from the addPredicate change.

This revision was automatically updated to reflect the committed changes.
dsanders added inline comments.Oct 13 2017, 5:59 PM
158 ↗(On Diff #118446)

I also renamed related functions like isTied().

2092 ↗(On Diff #118446)

I managed to make this work. addPredicate() now returns an optional and for the OperandMatcher will avoid adding more predicates when isSameAsAnotherOperand() is true.

The importer will now give up on a rule if isSameAsAnotherOperand() is true and it's asked to create a sub-instruction matcher. As far as I know, this situation never occurred but if it did it wouldn't have checked they were the same operand and would therefore have matched cases it shouldn't. It's technically possible to import this case but I don't think we should. It would occur for something like (add $src1:(sub $a, $b), $src1:(sub $a, $b)) but this only works if both $src1 sub-patterns are identical. If they differ, then matching is impossible and we ought to warn about that. However, rather than verify that the sub-patterns are identical, I think it makes more sense to encourage writing it as (add ($src1:(sub $a, $b), $src1). Without the redundancy, impossible rules can't occur.