This is an archive of the discontinued LLVM Phabricator instance.

Add optimization bisect opt-in calls for ARM passes
ClosedPublic

Authored by andrew.w.kaylor on Apr 22 2016, 5:35 PM.

Details

Summary

This patch adds calls to ARM-specific passes that can be safely skipped to opt-in to the optimization bisect mechanism.

I am not adding opt-in calls to the following passes, which appear to be required:

ARMConstantIslands
ARMExpandPseudo
Thumb2ITBlockPass

Note that the call to skipFunction() will also check for the "optnone" function attribute, so this can theoretically result in passes being skipped even when optimization bisect is not being done. However, I believe that any pass that can be safely skipped should be skipped for functions with the optnone attribute.

Diff Detail

Repository
rL LLVM

Event Timeline

andrew.w.kaylor retitled this revision from to Add optimization bisect opt-in calls for AArch64 passes.
andrew.w.kaylor updated this object.
andrew.w.kaylor added a reviewer: t.p.northover.
andrew.w.kaylor set the repository for this revision to rL LLVM.
andrew.w.kaylor added a subscriber: llvm-commits.
rengolin accepted this revision.Apr 23 2016, 6:04 AM
rengolin added a reviewer: rengolin.

IIRC, the three passes you mention are not for optimization, bot late code generation fixups.

From what I followed in your skipFunction() implementation, this is only enabled for debug purposes, to bisect and find problems, in which case, LGTM.

Thanks!

This revision is now accepted and ready to land.Apr 23 2016, 6:04 AM
andrew.w.kaylor retitled this revision from Add optimization bisect opt-in calls for AArch64 passes to Add optimization bisect opt-in calls for ARM passes.Apr 25 2016, 10:08 AM
andrew.w.kaylor edited edge metadata.

IIRC, the three passes you mention are not for optimization, bot late code generation fixups.

From what I followed in your skipFunction() implementation, this is only enabled for debug purposes, to bisect and find problems, in which case, LGTM.

Thanks!

The optnone aspect is controlled in the source via a pragma or attribute. Does that change your opinion?

Yes, the intention is to avoid skipping passes that are required for code generation, and this is only enabled for debugging purposes.

The "OptNone" attribute has different motivation than the opt bisect, but in both cases the optimizations are being disabled in response to direct user action.

Yes, the intention is to avoid skipping passes that are required for code generation, and this is only enabled for debugging purposes.

The "OptNone" attribute has different motivation than the opt bisect, but in both cases the optimizations are being disabled in response to direct user action.

No no no. Please do not mix up the two kinds of "users" here.
Bisection is for use by compiler developers to track down bugs in the optimizer.
Optnone is for use by end-users building their applications who want certain functions not to be optimized, usually as a tactic for improving debug info; this has nothing to do with debugging the compiler.

No no no. Please do not mix up the two kinds of "users" here.
Bisection is for use by compiler developers to track down bugs in the optimizer.
Optnone is for use by end-users building their applications who want certain functions not to be optimized, usually as a tactic for improving debug info; this has nothing to do with debugging the compiler.

That's what I meant by referring to the features having different motivation, and you're right that there are definitely two entirely different kinds of debugging involved. I just meant to emphasize that neither one does anything without the person using the compiler explicitly intending it.

No no no. Please do not mix up the two kinds of "users" here.
Bisection is for use by compiler developers to track down bugs in the optimizer.
Optnone is for use by end-users building their applications who want certain functions not to be optimized, usually as a tactic for improving debug info; this has nothing to do with debugging the compiler.

I assumed the bisection just applying optnone on selected functions (sequentially, randomly, GA, etc) until the function that has the bug manifests itself (if that simple).

In that case, the passes that were not skipped in this patch make a lot of sense, because they're required for correctness. In this case, the change looks good to me.

The fact that final users can *also* use this to disable not only the high-level optimisation passes, but also the low-level ones is an added bonus in my view.

It may prove to be problematic if some of those passes were accidentally triggering correctness code-gen, but that's something that the limited number of cases will tell us in the future. I can't see how one would know of all of them beforehand.

If that's a fair, description of the problem, than LGTM. :)

cheers,
--renato

As long as we all understand that

  • optnone is something that end-users will do for their own reasons, not as compiler developers; and
  • optnone should not be allowed to cause any correctness problems

then I'm okay. I have no idea about these passes specifically, I just don't want somebody approving a patch based on an incorrect assumption.

  • optnone is something that end-users will do for their own reasons, not as compiler developers; and

Yup.

  • optnone should not be allowed to cause any correctness problems

Yup.

then I'm okay. I have no idea about these passes specifically, I just don't want somebody approving a patch based on an incorrect assumption.

To the best of my knowledge, all those passes are optimisations only and should not have been used for anything other than that, either by design or accident. If they did, it's a serious unintentional bug.

Also, this is a simple patch, and if you do find anything on your side, just let us know and this can be easily and quickly reverted, but a bug needs to be created to address the issue.

cheers,
--renato

This revision was automatically updated to reflect the committed changes.
llvm/trunk/lib/Target/ARM/Thumb2SizeReduction.cpp
1028 ↗(On Diff #54920)

This was causing optnone-llc.ll to fail, so I'm taking it out, but it seems to me that this pass is definitely skippable. The optnone-llc.ll test fails because this pass is run at -O0. It seems to me that it shouldn't be, but I removed this to get the buildbot clean, since that seems like a higher priority.