[PATCH] D37443: [tablegen] Handle common load/store predicates inside tablegen. NFC.

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added inline comments.


================
Comment at: include/llvm/Target/TargetSelectionDAG.td:656
+  // They will be tested prior to the code in pred and must not be used in
+  // ImmLeaf and its subclasses.
+
----------------
qcolombet wrote:
> qcolombet wrote:
> > That comment reads like an easy way to shot ourselves in the foot :).
> > Could we add a check that when ImmediateCode is set, none of the following bits are set?
> At first, I thought it would be better to (painfully) recover this information from the patfrag name in the GI emitter, but looking at how ImmediateCode is handled, the proposed approach is fine.
> That comment reads like an easy way to shot ourselves in the foot :).

Yep, I'd like to make them unavailable in the *ImmLeaf subclasses but TableGen doesn't have a way to remove fields.

> Could we add a check that when ImmediateCode is set, none of the following bits are set?

Sure


================
Comment at: include/llvm/Target/TargetSelectionDAG.td:674
+  // cast<StoreSDNode>(N)->isTruncatingStore();
+  bit IsTruncStore = ?;
+
----------------
qcolombet wrote:
> Do we really need both NonTruncStore and TruncStore?
I should be able to fold them together. The reason I haven't is pretty weak. It's just that testing getValueAsBitOrUnset() returns false for the unset case so it was slightly simpler to only consider 1 or unset.


================
Comment at: include/llvm/Target/TargetSelectionDAG.td:679
+  // cast<LoadSDNode>(N)->getMemoryVT().getScalarType() == MVT::<VT>;
+  ValueType LoadScalarMemoryVT = ?;
+  // cast<StoreSDNode>(N)->getMemoryVT() == MVT::<VT>;
----------------
qcolombet wrote:
> Maybe have just one MemoryVT and one ScalarMemoryVT and something to check if that a store or load?
Ok


================
Comment at: utils/TableGen/CodeGenDAGPatterns.cpp:863
+        getOrigPatFragRecord()->getRecord()->getLoc(),
+        "IsUnindexedLoad and IsUnindexedStore are mutually exclusive");
+  if (isUnindexedLoad())
----------------
qcolombet wrote:
> If we have an unindexed bit and a load/store bit. Then this configuration wouldn't be possible.
Ok


https://reviews.llvm.org/D37443