Page MenuHomePhabricator

[CodeGen] Matching promoted type for 16-bit integer bitcasts from fp16 operand

Authored by pratlucas on Jun 25 2020, 7:34 AM.



When legalizing a biscast operation from an fp16 operand to an i16 on a
target that requires both input and output types to be promoted to
32-bits, an assertion can fail when building the new node due to a
mismatch between the the operation's result size and the type specified to
the node.

This patches fix the issue by making sure the bit width of the types
match for the FP_TO_FP16 node, covering the difference with an extra
ANYEXTEND operation.

Diff Detail

Event Timeline

pratlucas created this revision.Jun 25 2020, 7:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 25 2020, 7:34 AM

If you look at PromoteIntRes_FP_TO_FP16, it just widens the result type. I'm not sure why it isn't okay to do the same thing here; is there some interaction here I'm missing?

The issue happens when creating the ISD::FP_TO_FP16 in case the input is a constant.
It tries to generate the new 16-bit constant but the NOutVT type passed to DAG.getNode has 32-bits, which causes the assertion in SelectionDAG.cpp:1307 to fail:

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:1307: llvm::SDValue llvm::SelectionDAG::getConstant(const llvm::ConstantInt &, const llvm::SDLoc &, llvm::EVT, bool, bool): Assertion `Elt->getBitWidth() == EltVT.getSizeInBits() && "APInt size does not match type size!"' failed.

In that case, please fix SelectionDAG::getNode.

Is there anything blocking progress?

I'm waiting for my latest comment to be addressed.

pratlucas updated this revision to Diff 275135.Jul 2 2020, 8:34 AM

Moving fix to SelectionDAG::getNode.

plotfi added inline comments.Jul 2 2020, 10:40 AM




pratlucas updated this revision to Diff 275951.Jul 7 2020, 2:13 AM

Fixing typos.

pratlucas marked 2 inline comments as done.Jul 7 2020, 2:13 AM

@efriedma Does this seem right to you?

This revision is now accepted and ready to land.Jul 7 2020, 11:58 AM
This revision was automatically updated to reflect the committed changes.

@pratlucas Awesome thank you!