This is an archive of the discontinued LLVM Phabricator instance.

[ConstantFolding] Fix wrong folding of intrinsic 'convert.from.fp16'.
ClosedPublic

Authored by andreadb on May 14 2015, 7:57 AM.

Details

Summary

This patch fixes a wrong constant folding of intrinsic 'convert.from.fp16'.

Function 'ConstantFoldScalarCall' (in ConstantFolding.cpp) works under the wrong assumption that a call to 'convert.from.fp16' always returns a value of type 'float'.

However, intrinsic 'convert.from.fp16' can be 'overloaded'; for example, we can use 'convert.from.fp16.fp64' to convert from half to double; etc.

Before this patch, the following example would have triggered an assertion failure in opt (with '-constprop'):

define double @foo() {
  entry:
  %0 = call double @llvm.convert.from.fp16.f64(i16 0)
  ret double %0
}

in Value.cpp:325: void llvm::Value::replaceAllUsesWith(llvm::Value*): Assertion `New->getType() == getType() && "replaceAllUses of value with new value of different type!" failed.

This patch fixes the problem in ConstantFolding.cpp. When folding a call to convert.from.fp16, we perform a different kind of conversion based on the call return type.

Added test 'Transform/ConstProp/convert-from-fp16.ll'.

Please let me know if ok to submit.

Thanks!
-Andrea

Diff Detail

Event Timeline

andreadb updated this revision to Diff 25776.May 14 2015, 7:57 AM
andreadb retitled this revision from to [ConstantFolding] Fix wrong folding of intrinsic 'convert.from.fp16'..
andreadb updated this object.
andreadb edited the test plan for this revision. (Show Details)
andreadb added reviewers: hfinkel, majnemer.
andreadb added a subscriber: Unknown Object (MLST).
ab accepted this revision.May 14 2015, 8:48 AM
ab added a reviewer: ab.
ab added a subscriber: ab.

LGTM, thanks!

-Ahmed

lib/Analysis/ConstantFolding.cpp
1403

I swear I saw the same construct elsewhere, but I can't find it. Is there a good header where this could go?

test/Transforms/ConstProp/convert-from-fp16.ll
4–5

Since this means the code was untested before, what using a non-0 value? For the extension, the significand of the result should look the same, so we can get creative. (but there's no point in testing APFloat again; it's just that 0 is easy to get right the wrong way)

8

CHECK-LABEL?

This revision is now accepted and ready to land.May 14 2015, 8:48 AM
ab added inline comments.May 14 2015, 9:16 AM
lib/Analysis/ConstantFolding.cpp
1403

Ah, there's Type::getFltSemantics, please use that instead of re-defining it.

Thanks for the review!

lib/Analysis/ConstantFolding.cpp
1403

You are right!
I should have use getFltSemantics() from Type.h.
I will use it :-).

test/Transforms/ConstProp/convert-from-fp16.ll
4–5

Right, I can add more tests. My original test case had something like this:

%0 = call i16 @llvm.convert.to.fp16.f64(double 5.25)
%1 = call double @llvm.convert.from.fp16.f64(i16 %0)
ret double %1

I can add more tests like the above example.
I'll send a new patch soon.

8

I will add CHECK-LABELs.

andreadb updated this revision to Diff 25786.May 14 2015, 10:36 AM
andreadb edited edge metadata.

Hi Ahmed,
I updated the patch based on your previous feedback.

This patch is much simpler now. As you suggested, I also added extra tests for values different than zero.

Please let me know what you think.
Thanks!
-Andrea

ab added a comment.May 14 2015, 10:47 AM

Better indeed, go ahead!

-Ahmed

lib/Analysis/ConstantFolding.cpp
1549

Nit: I'd put the Ty->getFltSemantics() here instead of in the variable, but I'm fine with both ways.

This revision was automatically updated to reflect the committed changes.