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

[Diana Picus via Phabricator via llvm-commits]

rovka added a comment.

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



================
Comment at: utils/TableGen/CodeGenDAGPatterns.cpp:787
+    return "const APInt &";
+  else if (immCodeUsesAPFloat())
+    return "const APFloat &";
----------------
Redundant else.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:2555
+     << "enum {\n";
+  StringRef EnumeratorSeparator = " = GIPFP_Invalid,\n";
+  for (const auto *Record : RK.getAllDerivedDefinitions("PatFrag")) {
----------------
Shouldn't this be +1?


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:2568
+    if (!Record->getValueAsString("ImmediateCode").empty())
+      if (Filter(Record))
+        OS << "  static bool Predicate_" << Record->getName()
----------------
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.


https://reviews.llvm.org/D36534