Add optimization bisect opt-in calls for ARM passes

[andrew.w.kaylor]

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.

[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!

[probinson]

In D19449#410005, @rengolin wrote:

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?

[andrew.w.kaylor]

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.

[probinson]

In D19449#410859, @andrew.w.kaylor wrote:

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.

[andrew.w.kaylor]

In D19449#410913, @probinson wrote:

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.

[rengolin]

In D19449#410913, @probinson wrote:

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

[probinson]

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.

[rengolin]

In D19449#411264, @probinson wrote:

• 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

[andrew.w.kaylor]

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.

lib/Target/ARM/A15SDOptimizer.cpp

for (SmallVectorImpl<MachineInstr *>::iterator II = DefSrcs.begin(),
       }
       Replacements[MI] = NewReg;
     }
   }
   return Modified;
 }
 
 bool A15SDOptimizer::runOnMachineFunction(MachineFunction &Fn) {
+  if (skipFunction(*Fn.getFunction()))
+    return false;
+
   const ARMSubtarget &STI = Fn.getSubtarget<ARMSubtarget>();
   // Since the A15SDOptimizer pass can insert VDUP instructions, it can only be
   // enabled when NEON is available.
   if (!(STI.isCortexA15() && STI.hasNEON()))
     return false;
   TII = STI.getInstrInfo();
   TRI = STI.getRegisterInfo();
   MRI = &Fn.getRegInfo();

lib/Target/ARM/ARMLoadStoreOptimizer.cpp

if (Use.isKill()) {
       MBB.erase(Prev);
       return true;
     }
 
   llvm_unreachable("tMOVr doesn't kill a reg before tBX_RET?");
 }
 
 bool ARMLoadStoreOpt::runOnMachineFunction(MachineFunction &Fn) {
+  if (skipFunction(*Fn.getFunction()))
+    return false;
+
   MF = &Fn;
   STI = &static_cast<const ARMSubtarget &>(Fn.getSubtarget());
   TL = STI->getTargetLowering();
   AFI = Fn.getInfo<ARMFunctionInfo>();
   TII = STI->getInstrInfo();
   TRI = STI->getRegisterInfo();
 
   RegClassInfoValid = false;
namespace {
   };
   char ARMPreAllocLoadStoreOpt::ID = 0;
 }
 
 INITIALIZE_PASS(ARMPreAllocLoadStoreOpt, "arm-prera-load-store-opt",
                 ARM_PREALLOC_LOAD_STORE_OPT_NAME, false, false)
 
 bool ARMPreAllocLoadStoreOpt::runOnMachineFunction(MachineFunction &Fn) {
-  if (AssumeMisalignedLoadStores)
+  if (AssumeMisalignedLoadStores || skipFunction(*Fn.getFunction()))
     return false;
 
   TD = &Fn.getDataLayout();
   STI = &static_cast<const ARMSubtarget &>(Fn.getSubtarget());
   TII = STI->getInstrInfo();
   TRI = STI->getRegisterInfo();
   MRI = &Fn.getRegInfo();
   MF  = &Fn;

lib/Target/ARM/ARMOptimizeBarriersPass.cpp

static bool CanMovePastDMB(const MachineInstr *MI) {
   return !(MI->mayLoad() ||
           MI->mayStore() ||
           MI->hasUnmodeledSideEffects() ||
           MI->isCall() ||
           MI->isReturn());
 }
 
 bool ARMOptimizeBarriersPass::runOnMachineFunction(MachineFunction &MF) {
+  if (skipFunction(*MF.getFunction()))
+    return false;
+
   // Vector to store the DMBs we will remove after the first iteration
   std::vector<MachineInstr *> ToRemove;
   // DMBType is the Imm value of the first operand. It determines whether it's a
   // DMB ish, dmb sy, dmb osh, etc
   int64_t DMBType = -1;
 
   // Find a dmb. If we can move it until the next dmb, tag the second one for
   // removal

lib/Target/ARM/MLxExpansionPass.cpp

while (MII != E) {
 
     ++MII;
   }
 
   return Changed;
 }
 
 bool MLxExpansion::runOnMachineFunction(MachineFunction &Fn) {
+  if (skipFunction(*Fn.getFunction()))
+    return false;
+
   TII = static_cast<const ARMBaseInstrInfo *>(Fn.getSubtarget().getInstrInfo());
   TRI = Fn.getSubtarget().getRegisterInfo();
   MRI = &Fn.getRegInfo();
   const ARMSubtarget *STI = &Fn.getSubtarget<ARMSubtarget>();
   // Only run this for CortexA9.
   if (!STI->isCortexA9())
     return false;
   isLikeA9 = STI->isLikeA9() || STI->isSwift();

lib/Target/ARM/Thumb2SizeReduction.cpp

bool Thumb2SizeReduce::ReduceMBB(MachineBasicBlock &MBB) {
 
   MBBInfo &Info = BlockInfo[MBB.getNumber()];
   Info.HighLatencyCPSR = HighLatencyCPSR;
   Info.Visited = true;
   return Modified;
 }
 
 bool Thumb2SizeReduce::runOnMachineFunction(MachineFunction &MF) {
-  if (PredicateFtor && !PredicateFtor(*MF.getFunction()))
+  if (skipFunction(*MF.getFunction()) ||
+      PredicateFtor && !PredicateFtor(*MF.getFunction()))
     return false;
 
   STI = &static_cast<const ARMSubtarget &>(MF.getSubtarget());
   if (STI->isThumb1Only() || STI->prefers32BitThumb())
     return false;
 
   TII = static_cast<const Thumb2InstrInfo *>(STI->getInstrInfo());