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

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.

Looks mostly good to me.
Comments below.



================
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.
+
----------------
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?


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


================
Comment at: include/llvm/Target/TargetSelectionDAG.td:674
+  // cast<StoreSDNode>(N)->isTruncatingStore();
+  bit IsTruncStore = ?;
+
----------------
Do we really need both NonTruncStore and TruncStore?


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


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


https://reviews.llvm.org/D37443