This is an archive of the discontinued LLVM Phabricator instance.

Differential D5456

[ARM] [CodeGen] Do not emit intermediate register for zero FP immediate
ClosedPublic

Authored by sdmitrouk on Sep 23 2014, 12:32 AM.

Details

Reviewers
rengolin
t.p.northover
Summary

This updates check for double precision zero floating point constant to allow use of instruction with immediate value rather than temporary register.
Currently "a == 0.0", where "a" is of "double" type generates:

vmov.i32        d16, #0x0
vcmpe.f64       d0, d16

With this change it becomes:

vcmpe.f64        d0, #0

Diff Detail

Event Timeline

sdmitrouk updated this revision to Diff 13970.Sep 23 2014, 12:32 AM
sdmitrouk retitled this revision from to [ARM] [CodeGen] Do not emit intermediate register for zero FP immediate.
sdmitrouk updated this object.
sdmitrouk edited the test plan for this revision. (Show Details)
sdmitrouk added a reviewer: t.p.northover.
sdmitrouk set the repository for this revision to rL LLVM.
sdmitrouk added a subscriber: Unknown Object (MLST).
Herald added a subscriber: aemerson. · View Herald TranscriptSep 23 2014, 12:32 AM
sdmitrouk updated this revision to Diff 13972.Sep 23 2014, 12:34 AM

Used wrong diff (without full context) first time.

sdmitrouk added a comment.Oct 3 2014, 10:10 AM

Ping?

sdmitrouk added a comment.Oct 14 2014, 3:54 AM

Ping.

rengolin added a subscriber: rengolin.Oct 14 2014, 4:23 AM
rengolin added inline comments.
lib/Target/ARM/ARMISelLowering.cpp
3246

I'd say this is a bit too specific on what LowerConstantFP does today, which may change. Wouldn't any fp bitcast work in this case, if the value being casted is zero?

sdmitrouk added a comment.Oct 23 2014, 3:24 AM

Ping. Also, did my reply to line comment was sent? It has a strange "Not Submitted Yet" label on it.

lib/Target/ARM/ARMISelLowering.cpp
3246

Sure, it would work, but how to check node containing initial value for zero without going through all intermediate nodes? I was looking for a way to generalize this check, but didn't find it.

rengolin added a comment.Oct 23 2014, 4:04 AM

Hi Sergey,

The inline comments only go live when you click on "Submit" at the bottom of the page. This is to help you make many inline comments and only submit once. So, yeah, I only saw it now. :)

cheers,
--renato

lib/Target/ARM/ARMISelLowering.cpp
3246

I may be wrong, but wouldn't (Op->getValueType(0) == MVT::f32 || Op->getValueType(0) == MVT::f64) get all cases?

I don't expect vector types here, or half-floats...

sdmitrouk added a comment.Oct 23 2014, 5:03 AM

Hi Renato,

The thing with inline comments wasn't obvious for me :) I'm used to one email per comment.

Regards,
Sergey

lib/Target/ARM/ARMISelLowering.cpp
3246

I see it this way: if we need to check for immediate value the only way to do this is to walk through all intermediate nodes until we get to constant node containing the value.

Catching just zero immediate values or bitcasts to zero will ignore the one created by LowerConstantFP. It's a weird one, but the comment says "don't touch it for zero". isFloatingPointZero is called for arguments of comparison instructions to select special "compare with zero" form and zero argument is represented with such complex structure. I don't see a way to check for this "special zero" without hard-coding whole path to constant node somewhere.

rengolin accepted this revision.Oct 23 2014, 5:18 AM
rengolin added a reviewer: rengolin.

Right. Since you have a test, if this ever stops being true, we'll catch it. I won't bother with "potential other cases" until we know what they are.

LGTM, Thanks!
--renato

This revision is now accepted and ready to land.Oct 23 2014, 5:18 AM
sdmitrouk added a comment.Oct 23 2014, 8:09 AM

Thanks, Renato!

One more thing, could you please commit it?

Best regards,
Sergey

rengolin closed this revision.Oct 23 2014, 8:43 AM

Committed in r220486.

Thanks!

Revision Contents

PathSize
lib/
Target/
ARM/
12 lines
test/
CodeGen/
ARM/
12 lines

Diff 13972

lib/Target/ARM/ARMISelLowering.cpp

  • This file is larger than 256 KB, so syntax highlighting is disabled by default.
Show First 20 Lines • Show All 3,234 Lines • ▼ Show 20 Linesstatic bool isFloatingPointZero(SDValue Op) {
else if (ISD::isEXTLoad(Op.getNode()) || ISD::isNON_EXTLoad(Op.getNode())) { else if (ISD::isEXTLoad(Op.getNode()) || ISD::isNON_EXTLoad(Op.getNode())) {
// Maybe this has already been legalized into the constant pool? // Maybe this has already been legalized into the constant pool?
if (Op.getOperand(1).getOpcode() == ARMISD::Wrapper) { if (Op.getOperand(1).getOpcode() == ARMISD::Wrapper) {
SDValue WrapperOp = Op.getOperand(1).getOperand(0); SDValue WrapperOp = Op.getOperand(1).getOperand(0);
if (ConstantPoolSDNode *CP = dyn_cast<ConstantPoolSDNode>(WrapperOp)) if (ConstantPoolSDNode *CP = dyn_cast<ConstantPoolSDNode>(WrapperOp))
if (const ConstantFP *CFP = dyn_cast<ConstantFP>(CP->getConstVal())) if (const ConstantFP *CFP = dyn_cast<ConstantFP>(CP->getConstVal()))
return CFP->getValueAPF().isPosZero(); return CFP->getValueAPF().isPosZero();
} }
} else if (Op->getOpcode() == ISD::BITCAST &&
Op->getValueType(0) == MVT::f64) {
// Handle (ISD::BITCAST (ARMISD::VMOVIMM (ISD::TargetConstant 0)) MVT::f64)
// created by LowerConstantFP().
rengolinUnsubmitted
Not Done
ReplyInline Actions

I'd say this is a bit too specific on what LowerConstantFP does today, which may change. Wouldn't any fp bitcast work in this case, if the value being casted is zero?

rengolin: I'd say this is a bit too specific on what LowerConstantFP does today, which may change.
sdmitroukAuthorUnsubmitted
Not Done
ReplyInline Actions

Sure, it would work, but how to check node containing initial value for zero without going through all intermediate nodes? I was looking for a way to generalize this check, but didn't find it.

sdmitrouk: Sure, it would work, but how to check node containing initial value for zero without going…
rengolinUnsubmitted
Not Done
ReplyInline Actions

I may be wrong, but wouldn't (Op->getValueType(0) == MVT::f32 || Op->getValueType(0) == MVT::f64) get all cases?

I don't expect vector types here, or half-floats...

rengolin: I may be wrong, but wouldn't (Op->getValueType(0) == MVT::f32 || Op->getValueType(0) == MVT…
sdmitroukAuthorUnsubmitted
Not Done
ReplyInline Actions

I see it this way: if we need to check for immediate value the only way to do this is to walk through all intermediate nodes until we get to constant node containing the value.

Catching just zero immediate values or bitcasts to zero will ignore the one created by LowerConstantFP. It's a weird one, but the comment says "don't touch it for zero". isFloatingPointZero is called for arguments of comparison instructions to select special "compare with zero" form and zero argument is represented with such complex structure. I don't see a way to check for this "special zero" without hard-coding whole path to constant node somewhere.

sdmitrouk: I see it this way: if we need to check for immediate value the only way to do this is to walk…
SDValue BitcastOp = Op->getOperand(0);
if (BitcastOp->getOpcode() == ARMISD::VMOVIMM) {
SDValue MoveOp = BitcastOp->getOperand(0);
if (MoveOp->getOpcode() == ISD::TargetConstant &&
cast<ConstantSDNode>(MoveOp)->getZExtValue() == 0) {
return true;
}
}
} }
return false; return false;
} }
/// Returns appropriate ARM CMP (cmp) and corresponding condition code for /// Returns appropriate ARM CMP (cmp) and corresponding condition code for
/// the given operands. /// the given operands.
SDValue SDValue
ARMTargetLowering::getARMCmp(SDValue LHS, SDValue RHS, ISD::CondCode CC, ARMTargetLowering::getARMCmp(SDValue LHS, SDValue RHS, ISD::CondCode CC,
▲ Show 20 Lines • Show All 7,996 Lines • Show Last 20 Lines

test/CodeGen/ARM/fpcmp-f64-neon-opt.ll

  • This file was added.
; RUN: llc -mtriple=linux-arm-gnueabihf -mattr=+neon %s -o - | FileCheck %s
; Check that no intermediate integer register is used.
define i32 @no-intermediate-register-for-zero-imm(double %x) #0 {
entry:
; CHECK-LABEL: no-intermediate-register-for-zero-imm
; CHECK-NOT: vmov
; CHECK: vcmp
%cmp = fcmp une double %x, 0.000000e+00
%conv = zext i1 %cmp to i32
ret i32 %conv
}