[PATCH] D36534: [aarch64] Support APInt and APFloat in ImmLeaf subclasses and make AArch64 use them.

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added a comment.

Sorry for the slow reply, I was on holiday last week.

In https://reviews.llvm.org/D36534#856660, @rovka wrote:

> I can see why you need to change the patterns to be more general, but is the distinction between APInt, APFloat and 64-bit values really relevant? I looked a bit at the following patch which uses this for GlobalISel and I'm wondering if we could just get away with checking for each immediate a combination of isCImm, isFPImm and getBitWidth (so we wouldn't need to introduce different states for different kinds of immediates).
>
> I'm sure I'm missing some details, so what's the catch? :)


We can potentially remove int64_t later (see below) but we need to keep the distinction between APInt and APFloat. In SelectionDAG, the APInt kind corresponds to ConstantSDNode::getAPIntValue() while the APFloat kind corresponds to ConstantFPSDNode::getValueAPF(). SelectionDAG needs to know which to use when emitting the predicate code. We could generate code to figure it out but we'd be doing work at run-time that we can easily do at compile-time. Similarly, GlobalISel needs to know whether to use ConstantInt::getValue() or ConstantFP::getValueAPF() and knows which one during tablegen so it makes sense to do this at compile-time.

The reason I think we can remove int64_t is that it's equivalent to an APInt predicate where `Imm` is always accessed with `Imm->getSExtValue()`. I haven't done this because it requires a change to every target.



================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:2555
+     << "enum {\n";
+  StringRef EnumeratorSeparator = " = GIPFP_Invalid,\n";
+  for (const auto *Record : RK.getAllDerivedDefinitions("PatFrag")) {
----------------
rovka wrote:
> Shouldn't this be +1?
Yes, it should. I've already fixed a similar issue upstream so it should be fixed in my next rebase.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:2568
+    if (!Record->getValueAsString("ImmediateCode").empty())
+      if (Filter(Record))
+        OS << "  static bool Predicate_" << Record->getName()
----------------
rovka wrote:
> Could you get rid of this repetition? If we don't expect many of these, we could probably use a copy_if into another container, but if you think that might affect performance too much you can probably just extract a ForEachFilteredImm lambda/helper. I can think of other variations but I'll let you choose whatever you think is best.
Ok


https://reviews.llvm.org/D36534