Page MenuHomePhabricator

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

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

Details

Summary

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

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.

include/llvm/CodeGen/GlobalISel/InstructionSelector.h
158

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 :).

test/CodeGen/X86/GlobalISel/select-blsr.mir
16

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.

utils/TableGen/GlobalISelEmitter.cpp
2040

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

2041

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
include/llvm/CodeGen/GlobalISel/InstructionSelector.h
158

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.

test/CodeGen/X86/GlobalISel/select-blsr.mir
16

Ok

utils/TableGen/GlobalISelEmitter.cpp
2040

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

2041

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
include/llvm/CodeGen/GlobalISel/InstructionSelector.h
158

I also renamed related functions like isTied().

utils/TableGen/GlobalISelEmitter.cpp
2041

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.