Page MenuHomePhabricator

[IR] Redefine Freeze instruction
ClosedPublic

Authored by aqjune on Nov 6 2019, 11:31 PM.

Details

Summary

This patch redefines freeze instruction from being UnaryOperator to a subclass of UnaryInstruction.

ConstantExpr freeze is removed, as discussed in the previous review.
FreezeOperator is not added because there's no ConstantExpr freeze.
freeze i8* null test is added to test/Bindings/llvm-c/freeze.ll as well, because the null pointer-related bug in tools/llvm-c/echo.cpp is now fixed.
InstVisitor has visitFreeze now because freeze is not unaryop anymore.

Diff Detail

Event Timeline

aqjune created this revision.Nov 6 2019, 11:31 PM
Herald added a project: Restricted Project. · View Herald Transcript
aqjune marked an inline comment as done.Nov 6 2019, 11:40 PM
llvm/include/llvm/IR/IRBuilder.h
2396

FreezeInst::Create is not used here because its argument needs no special check.

lebedev.ri accepted this revision.Nov 10 2019, 9:44 AM

Thanks! Sorry for double work..

I guess in principle this has backwards compatibility issues,
since bitc::UNOP_FREEZE goes away? But since LLVM itself
never produced them until this moment, do we care?

Other than that this looks good to me.
Anyone else?

This revision is now accepted and ready to land.Nov 10 2019, 9:44 AM
craig.topper accepted this revision.Nov 11 2019, 2:17 PM

Thanks! Sorry for double work..

I guess in principle this has backwards compatibility issues,
since bitc::UNOP_FREEZE goes away? But since LLVM itself
never produced them until this moment, do we care?

Other than that this looks good to me.
Anyone else?

I don't think we have a backwards compatibility guarantee on trunk. Only across release boundaries. So I think this should be fine.

LGTM

Thanks! Sorry for double work..

I guess in principle this has backwards compatibility issues,
since bitc::UNOP_FREEZE goes away? But since LLVM itself
never produced them until this moment, do we care?

Other than that this looks good to me.
Anyone else?

I don't think we have a backwards compatibility guarantee on trunk. Only across release boundaries. So I think this should be fine.

Hm, good to know, thanks!

LGTM

This revision was automatically updated to reflect the committed changes.

Sorry for the delayed response, but the OCaml test is failing: http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/19714/steps/test-check-all/logs/stdio

Could you take a look please?

Hi, sorry I'll fix it soon, perhaps in an hour or so.