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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added inline comments.


================
Comment at: include/llvm/Target/TargetSelectionDAG.td:697
+  let IsAPInt = 1;
+  let FastIselShouldIgnore = 1;
+}
----------------
bjope wrote:
> The comment above says that this the same as ImmLeaf except that Imm is an APInt. But you have also changed the default setting for FastIselShouldIgnore. So if that matters for some target you can't just replace ImmLeaf by IntImmLeaf.
> 
> I actually do not get the comments in the summary about the change being NFC for FastISel due to this. You changed lots of  AArch64 definitions to use IntImmLeaf/FPImmLeaf instead of using PatLeaf. But I guess we could see PatLeaf as having FastISelShouldIgnore=0 (because there is no ignore in FastISel for PatLeaf, right?). So to me it looks like you have made changes to how FastISel will handle all those definitions that you have updated to use IntImmLeaf/FPImmLeaf.
> 
> I don't know much about FastISel so maybe I misunderstand something. And maybe AArch64 doesn't ever use FastISel. If that is the case then your patch could be seen as NFC, but then I still don't get the point in why it was important to set FastIselShouldIgnore = 1 here.
> The comment above says that this the same as ImmLeaf except that Imm is an APInt. But you have
> also changed the default setting for FastIselShouldIgnore. So if that matters for some target you can't
> just replace ImmLeaf by IntImmLeaf.

For SelectionDAG and GlobalISel an IntImmLeaf is the same as an ImmLeaf except that the Imm variable is an APInt. This would also be the case for FastISel but FastISel doesn't know how to generate predicates that use APInt at the moment.

> I actually do not get the comments in the summary about the change being NFC for FastISel due to 
> this. You changed lots of AArch64 definitions to use IntImmLeaf/FPImmLeaf instead of using PatLeaf. > But I guess we could see PatLeaf as having FastISelShouldIgnore=0 (because there is no ignore in 
> FastISel for PatLeaf, right?). So to me it looks like you have made changes to how FastISel will handle 
> all those definitions that you have updated to use IntImmLeaf/FPImmLeaf.
> 
> I don't know much about FastISel so maybe I misunderstand something. And maybe AArch64 doesn't
> ever use FastISel. If that is the case then your patch could be seen as NFC, but then I still don't get the 
> point in why it was important to set FastIselShouldIgnore = 1 here.

FastISelEmitter.cpp doesn't have any code to import patterns from PatFrag/PatLeaf so they are effectively treated as FastIselShouldIgnore=1. The main issue preventing them from being imported into FastISel seems to be the same one that affects GlobalISel which is that the C++ in the predicates is written to use SDNode objects but those don't exist in FastISel/GlobalISel.

In the long term, FastISel could be extended to support APInt/APFloat predicates at which point we can drop the FastIselShouldIgnore=1.


https://reviews.llvm.org/D36534