This is an archive of the discontinued LLVM Phabricator instance.

[aarch64] Support APInt and APFloat in ImmLeaf subclasses and make AArch64 use them.
ClosedPublic

Authored by dsanders on Aug 9 2017, 10:30 AM.

Details

Summary

The purpose of this patch is to expose more information about ImmLeaf-like
PatLeaf's so that GlobalISel can learn to import them. Previously, ImmLeaf
could only be used to test int64_t's produced by sign-extending an APInt.
Other tests on immediates had to use the generic PatLeaf and extract the
constant using C++.

With this patch, tablegen will know how to generate predicates for APInt,
and APFloat. This will allow it to 'do the right thing' for both SelectionDAG
and GlobalISel which require different methods of extracting the immediate
from the IR.

This is NFC for SelectionDAG since the new code is equivalent to the
previous code. It's also NFC for FastISel because FastIselShouldIgnore is 1
for the ImmLeaf subclasses. Enabling FastIselShouldIgnore == 0 for these new
subclasses will require a significant re-factor of FastISel.

For GlobalISel, it's currently NFC because the relevant code to import the
affected rules is not yet present. This will be added in a later patch.

Depends on D36086

Event Timeline

dsanders created this revision.Aug 9 2017, 10:30 AM
dsanders edited the summary of this revision. (Show Details)Aug 9 2017, 2:36 PM

Removed the reference to zero-extending APInt's from the summary since this isn't in the patch anymore. IntImmLeaf with a predicate that uses Imm->getZExtValue() is equivalent so I didn't see a need to make uint64_t predicates separately.

rovka edited edge metadata.Aug 30 2017, 7:44 AM

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

utils/TableGen/CodeGenDAGPatterns.cpp
787

Redundant else.

utils/TableGen/GlobalISelEmitter.cpp
2507

Shouldn't this be +1?

2520

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.

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

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.

utils/TableGen/GlobalISelEmitter.cpp
2507

Yes, it should. I've already fixed a similar issue upstream so it should be fixed in my next rebase.

2520

Ok

dsanders updated this revision to Diff 113769.Sep 4 2017, 9:21 AM
dsanders marked 5 inline comments as done.

Rebase and fix nits

javed.absar added inline comments.Sep 4 2017, 11:47 AM
include/llvm/Target/TargetSelectionDAG.td
685

I am no expert on this, but would the comment be better as 'Is the data type... "
The 'Should the data... " seems to suggest an uncertainty (not sure), which is not the case here.

dsanders added inline comments.Sep 5 2017, 2:08 AM
include/llvm/Target/TargetSelectionDAG.td
685

Both make sense to me since in both cases tablegen is asking the question of the user and the user supplies the answer in the field. If there's a preference for a particular phrasing then I'm happy to change it.

bjope added a subscriber: bjope.Sep 5 2017, 1:37 PM
bjope added inline comments.
include/llvm/Target/TargetSelectionDAG.td
697

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.

dsanders added inline comments.Sep 5 2017, 2:50 PM
include/llvm/Target/TargetSelectionDAG.td
697

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.

bjope added inline comments.Sep 5 2017, 11:31 PM
include/llvm/Target/TargetSelectionDAG.td
697

FastISelEmitter.cpp doesn't have any code to import patterns from PatFrag/PatLeaf so they are effectively treated as FastIselShouldIgnore=1.

Ok I see. That explains why this is NFC also for FastISel. Because you have replaced a couple of PatLeaf by IntImmLeaf in this patch.

But I guess that FastISelEmitter.cpp has support for importing ImmLeaf patterns. So it is still confusing to say that IntImmLeaf is the same as an ImmLeaf except the Imm being an APInt. It is also different when it comes to FastIselShouldIgnore setting. So replacing ImmLeaf by IntImmLeaf would not be an NFC change. Maybe not a big deal, but that code comment fooled me for awhile until.

dsanders updated this revision to Diff 118409.Oct 10 2017, 9:58 AM

Rebased to trunk. I'll fix the comments I've received shortly.

This patch now requires r315148 to be reverted. That patch changed
getPredCode() and getImmCode() to return StringRef's but this patch requires
that getPredCode() return a std::string. We could revert just the getPredCode()
portion of that patch if we want but I think it would be weird for the two
functions to require different usage.

dsanders updated this revision to Diff 118412.Oct 10 2017, 10:08 AM

'Should the ... be ...?' comments changed to 'Is the ... a ...?'
Clarified that IntImmLeaf is not a replacement for ImmLeaf but rather a
replacement for PatLeaf with immediate predicates that ImmLeaf can't handle.

Rebased to trunk. I'll fix the comments I've received shortly.

This patch now requires r315148 to be reverted. That patch changed
getPredCode() and getImmCode() to return StringRef's but this patch requires
that getPredCode() return a std::string. We could revert just the getPredCode()
portion of that patch if we want but I think it would be weird for the two
functions to require different usage.

I'm just curious, what's the issue with it returning a StringRef?

Rebased to trunk. I'll fix the comments I've received shortly.

This patch now requires r315148 to be reverted. That patch changed
getPredCode() and getImmCode() to return StringRef's but this patch requires
that getPredCode() return a std::string. We could revert just the getPredCode()
portion of that patch if we want but I think it would be weird for the two
functions to require different usage.

I'm just curious, what's the issue with it returning a StringRef?

getPredCode() needs to build the code from several substrings depending on the data type of the ImmLeaf subclass. StringRef's don't own the data they reference so it will end up referencing a local that's destroyed by the exit from getPredCode().

This revision is now accepted and ready to land.Oct 12 2017, 11:17 AM
dsanders closed this revision.Oct 13 2017, 1:42 PM