Added the integer data processing intrinsics from Chapter 9 of the ACLE v2.1 and modified existing ones to include a chain if they write to either the Q or GE flags of the APSR.
The document can be found at: http://infocenter.arm.com/help/topic/com.arm.doc.ihi0053d/IHI0053D_acle_2_1.pdf
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
Hi Sam,
This is a big patch, with lots of smaller changes within. Normally, I'd ask you to split into smaller patches, but realistically, all of the changes have the exact same effect, are reasonably obvious and common. Your new tests also show that they work as expected, without breaking old assumptions (no old tests changed). From that perspective, it looks good to me, but I'll let it simmer for a bit, if someone else has some more comments.
Note: I'm assuming the "IntrNoMem" declaration in some intrinsics have to do with the "HasSideEffects" on the same bundle, but I haven't checked every single instruction to make sure they are strictly correlated.
cheers,
--renato
test/CodeGen/ARM/smul.ll | ||
---|---|---|
294 ↗ | (On Diff #95909) | Is this really necessary? |
I'm not convinced you're handling the Q and GE flags correctly for all of the intrinsics which set/use them. If we allow users to access those flags, every instruction which might set them needs to be mapped to a SelectionDAG node with a chain, every instruction which sets or uses them needs to be marked hasSideEffects, and we can never turn arithmetic into an operation which sets Q or GE. I'm not convinced you've managed that correctly everywhere. We currently have an ARMISD::SSAT node which doesn't have a chain. We have a pattern which generates SMLABB from an ISD::MUL. And it's difficult to review the rest because you're adding them all at once.
include/llvm/IR/IntrinsicsARM.td | ||
---|---|---|
87 ↗ | (On Diff #95917) | IntrReadMem? |
lib/Target/ARM/ARMInstrInfo.td | ||
877 ↗ | (On Diff #95917) | ImmLeaf? |
Hi Eli,
As far as I'm aware, these instructions do not read the Q flag, and I discussed why I haven't added the intrinsic that would enable the user to read that flag in D32282. This obviously doesn't stop the user from doing so in inline assembly though. As for the GE flags, I believe the only instruction that reads that flag is sel which is only selected via the intrinsic. So if I've got these descriptions correct, I think the chaining should be good. I will double check my descriptions though.
cheers,
sam
I had missed a few instructions that write to the GE flag and have now also set the sel intrinsic to IntrReadMem.
Either we're trying to support reading the Q flag, or we're not trying to support it. If we are trying to support it, we should do it correctly; if we aren't trying to support it, we might as well mark the intrinsics which write to it as IntrNoMem. There isn't really a middle ground here.
Also, I'd like to see comments on each of the intrinsics which isn't IntrNoMem describing which bits it reads/writes.
lib/Target/ARM/ARMISelLowering.cpp | ||
---|---|---|
3382 ↗ | (On Diff #96369) | You might want to add a comment noting that there's a bit of magic to handle this in LegalizeTypes; it looks weird to replace a node which returns i64 with a node that returns (i32, i32). |
lib/Target/ARM/ARMInstrInfo.td | ||
5736 ↗ | (On Diff #96369) | ARMV5TEPat? Same in other places; using plain "Pat" is a bad idea because we can end up with the wrong instruction in Thumb mode (and your tests wouldn't catch this because you don't check the encoding). |
877 ↗ | (On Diff #95917) | ImmLeaf? |
Fixed up the ARM patterns to use predicates, which also showed that I had missed several patterns for the Thumb versions! These have now been added.
lib/Target/ARM/ARMInstrInfo.td | ||
---|---|---|
5736 ↗ | (On Diff #96369) | ah yes, thanks! |
lib/Target/ARM/ARMInstrThumb2.td | ||
---|---|---|
2851 ↗ | (On Diff #96687) | t2SMLALD? (Same in other places.) |
Thanks Eli, I've now corrected the multiplication intrinsic patterns to select the Thumb version of the instruction and also added thumbv7em as a target to the test.
It would be nice to have an ARMV5TE test to make sure that the intrinsics that are supposed to work for that target actually do.
lib/Target/ARM/ARMISelLowering.cpp | ||
---|---|---|
7795 ↗ | (On Diff #96913) | Assuming that LowerINTRINSIC_WO_CHAIN returns a value with exactly two results seems a little dubious; it might work here, but it's a landmine for anyone dealing with this code in the future. Better to just make it a separate method which pushes onto the Results vector directly. |
Hi Eli,
I've created a standalone function to lower the i64 intrinsics because, yes it was pretty horrible before. The V5TE compatible intrinsics have now been separated out into another test file.
cheers,