Page MenuHomePhabricator

[SimplifyLibCalls][IRBuilder] Accept any IRBuilder in SimplifyLibCalls
ClosedPublic

Authored by nikic on Feb 18 2020, 1:51 PM.

Details

Summary

This changes the SimplifyLibCalls utility to accept an IRBuilderBase, which allows us to pass through the IRBuilder used by InstCombine. This will ensure that new instructions get added to the worklist. The annotated test-case drops from 4 to 2 InstCombine iterations thanks to this.

To achieve this, I'm adding an IRBuilderBase::OperandBundlesGuard, which is basically the same as the existing InsertPointGuard and FastMathFlagsGuard, but for operand bundles. Also add a setDefaultOperandBundles() method so these can be set outside the constructor.

Diff Detail

Event Timeline

nikic created this revision.Feb 18 2020, 1:51 PM

That's a nice improvement. I do have one detail question.

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
3109–3111

The OperandBundlesGuard covers this point, where bundles where previously not applied as far as I can tell. Is that intended?

nikic marked an inline comment as done.Feb 19 2020, 7:00 AM
nikic added inline comments.
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
3109–3111

It looks like the separate builder was introduced in https://github.com/llvm/llvm-project/commit/b491a2d641add36f1a2f7674d6b5df038c2d638e to fix the insertion position, and the later introduction of operand bundle handling in https://github.com/llvm/llvm-project/commit/b70e23c3909ca9a848a7ba963ee6580f48c57c5f forgot to adjust this separate builder. I don't think that operand bundles were intentionally dropped here. (Though most likely nobody is using operand bundles on fortified library calls in the first place.)

This revision is now accepted and ready to land.Feb 21 2020, 3:47 AM
This revision was automatically updated to reflect the committed changes.

Hi,

Is this supposed to affect the output from instcombine?

When I merged this to our downstream clone I noticed that it does.
If I run instcombine on the following input:

declare i32 @strlen(i8*)

define void @test_it() {
bb1:
  %_tmp12 = call i32 @strlen(i8* undef)
  ret void
}

with

opt -instcombine -S -o - strlen.ll

I got

define void @test_it() {
bb1:
  ret void
}

before, but with this patch I now get

define void @test_it() {
bb1:
  store i8 undef, i8* null, align 536870912
  ret void
}

instead.

nikic added a comment.Feb 24 2020, 1:30 AM

@uabelho As this patch changes worklist order, that can also have an impact on the optimization result. For your example, the strlen is folded to zext of load. If the zext is DCEd first, the load is also DCEd (before this patch) and you end up with an empty function. If the load is processed first, it will fold to unreachable instead (after this patch). D75008 will change the order again to perform DCE first, in which case the previous behavior will be restored. Both outcomes are generally fine though.

@nikic Alright, thanks!