[clang] [compiler-rt] [llvm] [SystemZ] Add support for half (fp16) (PR #109164)
Ulrich Weigand via cfe-commits
cfe-commits at lists.llvm.org
Mon Apr 7 05:42:18 PDT 2025
uweigand wrote:
> > I see that the LowerOperationWrapper still emits i128 operations. (E.g. you expand a f16->i128 into a f16->f32 and f32->i128) But that routine is called because of the illegal input type i128, so I understand it must not leave any operations with the illegal type; rather, it should complete the expansion fully.
>
> My understanding is that the TypeLegalizer calls here for i128 operands/results for Custom operations, even though the target doesn't have to do anything. If Results is returned empty, TypeLegalizer will try to handle it. New nodes in Results on the other hand will be visited as well.
>
> > This is annoying since for legal i128 we do the expansion in LowerOperation while for illegal i128 we do it in LowerOperationWrapper..
>
> I followed your example of calling LowerOperationWrapper instead to avoid the code duplication - looks better.
>
That's not quite what I was thinking. You now call into LowerOperationWrapper for f16 always - but f16 is always legal so there shouldn't be a need to do that. Instead, the type that is sometimes legal and sometimes not is i128 - so I would have thought the right way to call into LowerOperationWrapper for *i128* always. (For example, when i128 is not legal, where is the libcall even emitted now? LowerOperation shouldn't be called for legal types, and your current LowerOperationWrapper doesn't emit libcalls?)
> Added new opcodes for half per what was previously done for float and double. Special handling needed to remove the fpround for f128 as it would otherwise be lowered to a libcall. (The fpround for f128 could be removed for float and double as well, but there are currently no tests for this).
This seems to be more suitable for a DAGCombiner rule as it is really a performance optimization. This could also be done as a separate patch (for all the types).
https://github.com/llvm/llvm-project/pull/109164
More information about the cfe-commits
mailing list