This is an archive of the discontinued LLVM Phabricator instance.

[DAGCombiner] Add support for FCEIL, FFLOOR and FTRUNC vector constant folding
ClosedPublic

Authored by RKSimon on Mar 30 2015, 5:01 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

RKSimon updated this revision to Diff 22918.Mar 30 2015, 5:01 PM
RKSimon retitled this revision from to [DAGCombiner] Add support for FCEIL, FFLOOR and FTRUNC vector constant folding.
RKSimon updated this object.
RKSimon edited the test plan for this revision. (Show Details)
RKSimon added reviewers: qcolombet, mkuper, andreadb.
RKSimon set the repository for this revision to rL LLVM.
RKSimon added a subscriber: Unknown Object (MLST).
qcolombet added inline comments.Apr 3 2015, 11:06 AM
lib/CodeGen/SelectionDAG/SelectionDAG.cpp
2848

Naive question: Is it possible to have a BuildVector without a VT for a vector?

test/CodeGen/X86/vec_floor.ll
228

Could it be possible to add negative test when the conversion are not precise enough?

RKSimon added inline comments.Apr 3 2015, 11:15 AM
lib/CodeGen/SelectionDAG/SelectionDAG.cpp
2848

Yes - x86_mmx registers choke on VT.getVectorElementType() if we don't have the VT.isVector() test to stop them.

test/CodeGen/X86/vec_floor.ll
228

Yes I should be able to get something to work - probably with explicit hex values though.

RKSimon added inline comments.Apr 3 2015, 11:59 AM
test/CodeGen/X86/vec_floor.ll
228

I haven't found anything that works yet - do you known of any equivalent values in other tests? I haven't found anything so far.

qcolombet added inline comments.Apr 3 2015, 2:27 PM
lib/CodeGen/SelectionDAG/SelectionDAG.cpp
2848

Could you add a test case for that?

test/CodeGen/X86/vec_floor.ll
228

I guess you can check for FLT_MAX before doing a ceil and FLT_MIN for a floor.
Same for double.
But to be honest, I do not know better.

RKSimon updated this revision to Diff 23268.Apr 6 2015, 4:46 AM
RKSimon updated this object.

I've refreshed this patch after realising that I was being a little over the top with the original implementation:

1 - we no longer need to test for the output type being a vector - this can only occur from the folded unary op being a BITCAST (to a non vector type) and no code path requires BITCASTs to treat the VT as a vector anymore. There are plenty of existing test cases that do this (vec -> mmx, vec -> i64 vec -> i128 are examples I found).

2 - we can safely use the existing scalar path for FCEIL/FFLOOR/FTRUNC ops (and avoid all that functional duplication) - if they do fail to fold they will return a FCEIL/FFLOOR/FTRUNC scalar op which will fail the vector test for a UNDEF/CONSTANT/CONSTANTFP result.

3 - despite various suggestions I've not found any floating point constant that fails on the APFloat roundToIntegral - given the patch now uses the existing scalar path that already thoroughly tests this I think we can safely just use that and just accept that its being very, very cautious. I still wonder if its possible to cause it to fail with more exotic float types but haven't found any examples in the DAGCombine or InstCombine float constant folding tests.

qcolombet accepted this revision.Apr 6 2015, 10:02 AM
qcolombet edited edge metadata.

Hi Simon,

LGTM.

Thanks,
-Quentin

This revision is now accepted and ready to land.Apr 6 2015, 10:02 AM

Thanks Quentin - sorry for the false starts with this one.

This revision was automatically updated to reflect the committed changes.