Page MenuHomePhabricator

DAG: Tolerate non-MemSDNOdes for OPC_RecordMemRef
ClosedPublic

Authored by arsenm on Dec 11 2017, 11:56 AM.

Details

Summary

When intrinsics are allowed to have mem operands (D40978), there
are two ways this can happen. First is an intrinsic
that is marked has having a mem operand, but is not handled
by getTgtMemIntrinsic.

The second way can occur even for intrinsics which do not
have a mem operand. It seems the selector table does
some kind of sorting based on the opcode, and the
mem ref recording can happen in the same scope for
intrinsics that both do and do not have mem refs.
I haven't been able to figure out exactly why this happens
(although it happens even with the matcher optimizations disabled).
I'm not sure if it's worth trying to avoid hitting this for
these nodes since I think it's still reasonable to handle
this in case getTgtMemIntrinic is not implemented.

Diff Detail

Event Timeline

arsenm created this revision.Dec 11 2017, 11:56 AM

Do you have a test case? I don't understand from your description what's happening. Do you have a target intrinsic with an ID in the wrong range (i.e., not > FIRST_TARGET_MEMORY_OPCODE)?

Do you have a test case? I don't understand from your description what's happening. Do you have a target intrinsic with an ID in the wrong range (i.e., not > FIRST_TARGET_MEMORY_OPCODE)?

As it is now, if you have a raw intrinsic (i.e. ISD::INTRINSIC_*) node, the memory operand is always dropped. D40978 allows preserving it by setting the property on the intrinsic, so OPC_RecordMemRef will actually be encountered. If you have an intrinsic marked as having a memory operand, but getTgtMemIntrinsic does not handle it, it won't be a MemSDNode. The other case is probably a table gen bug not understanding different memory properties when the same opcode is used, but I'm not sure how worth it is continuing to investigate that.

Okay. Can you add a DEBUG statement saying something about when we're dropping the memory operand?

arsenm updated this revision to Diff 127373.Dec 18 2017, 8:57 AM

Add some debug printing

hfinkel accepted this revision.Dec 19 2017, 9:29 PM

LGTM

This revision is now accepted and ready to land.Dec 19 2017, 9:29 PM
arsenm closed this revision.Dec 20 2017, 11:12 AM

r321208