LLVM 3.9 RC2's SCCP pass removing calls to external functions?!

Hi,

I’ve been porting a project to LLVM 3.9 and found that the SCCP pass may remove calls to external functions if their return type is declared to be an empty struct. For instance:

%empty = type {}

declare %empty @foo()

define i32 @main() {
%1 = call %empty @foo()
ret i32 0
}

opt -sccp -S file.ll returns:

%empty = type {}

declare %empty @foo()

define i32 @main() {
ret i32 0
}

It doesn’t look right to remove a call to an external function (especially since the call isn’t removed if @foo returns a void instead). It didn’t happen with LLVM 3.8.

I’m using the Mac OS X prebuilt binaries for RC2.

Assuming that this is a bug, what are the next steps?

Félix

Hi Félix,

Hi Félix,

Sanjoy Das wrote:

Btw, looks like the bug is in runSCCP in

    for (BasicBlock::iterator BI = BB.begin(), E = BB.end(); BI != E;) {
      Instruction *Inst = &*BI++;
      if (Inst->getType()->isVoidTy() || isa<TerminatorInst>(Inst))
        continue;

      if (tryToReplaceInstWithConstant(Solver, Inst,
                                       true /* shouldEraseFromParent */)) {
        // Hey, we just changed something!
        MadeChanges = true;
        ++NumInstRemoved;
      }

It should not be passing in true for shouldEraseFromParent. I think
the right fix here is to not have the shouldEraseFromParent parameter
at all, but in tryToReplaceInstWithConstant to do:

  // replace Inst with constant
  llvm::RecursivelyDeleteTriviallyDeadInstructions(Inst, TLI);

Let me know if you want to take a crack at this, otherwise I'll get to
it this week.

-- Sanjoy

I am probably stating the obvious, but if the function is side-effect free (onlyReadsMemory) it is valid to remove it.

But I am guessing that does not belong to this pass.

I think it does.

This is a very aggressive pass that does all sorts of inter-procedural
cleanups, and unused side-effect free function calls is just the
thing.

It looks to me that the compiler just got a bit more picky and your
original assumption was wrong to begin with.

Maybe your project needs to tell the middle-end (via function
attributes) that the function does have side effects?

cheers,
--renato

> I am probably stating the obvious, but if the function is side-effect
free
> (onlyReadsMemory) it is valid to remove it.
>
> But I am guessing that does not belong to this pass.

I think it does.

This is a very aggressive pass that does all sorts of inter-procedural
cleanups, and unused side-effect free function calls is just the
thing.

I see. Then I agree.

It looks to me that the compiler just got a bit more picky and your
original assumption was wrong to begin with.

However, he also stated that in the case the functions returns void it is
not removed, which suggest there is something fishier going on.

Looking back at the example (and LangRef, for function attributes),
looks like it's not legal to remove because the function does *not*
have "readnone" or "readonly", so LLVM can't prove it side-effect
free.

Sanjoy's fix is probably on the right track.

--renato

Yes, there is no way for any pass to find out by itself whether that function has side effects, and I didn’t tell it that it doesn’t have any, so that seems overly aggressive.

As far as finding and fixing the problem goes: I used to build LLVM and Clang from source, but I stopped because a full build takes around 3 hours on this machine and I don’t get a lot out of it. I use LLVM on my spare time and I’m about as casual as a compiler back-end user can be. Sanjoy, I’d be happy if you looked into it.

Félix

Yes, there is no way for any pass to find out by itself whether that
function has side effects, and I didn't tell it that it doesn't have any, so
that seems overly aggressive.

Indeed.

As far as finding and fixing the problem goes: I used to build LLVM and
Clang from source, but I stopped because a full build takes around 3 hours
on this machine and I don't get a lot out of it. I use LLVM on my spare time
and I'm about as casual as a compiler back-end user can be. Sanjoy, I'd be
happy if you looked into it.

I've just done it locally and it works on your example, but has some
test failures.

I've open a bug: https://llvm.org/bugs/show_bug.cgi?id=29120, and made
it block 3.9.0 for now.

cheers,
--renato

Actually, this is false.
It will infer the attribute :slight_smile:

(though it’s still illegal to remove it in this particular case).

I’m just saying, just because you don’t make a function read-only or read-none, doesn’t mean clang/llvm won’t infer the attribute.

Here, it’s illegal because you don’t have the definition.

The function is also may-throw.
So even if it was readnone/readonly, we could not remove the call.

This case also fails when the function is "extern".

cheers,
--renato