[CodeGen] Add `shouldDoPartialRedundancyElimination()` to `TargetInstrInfo` (PRR42405)

[anton-afanasyev]

Summary

For particular subtargets setting shouldDoPartialRedundancyElimination() to true
prevents MIR code from SimplePRE optimization.

https://bugs.llvm.org/show_bug.cgi?id=42405

[lebedev.ri]

Test coverage missing.

[anton-afanasyev]

Update comment

[igorb]

LGTM

[lebedev.ri]

I feel like this is lacking words.
What problem is this addressing?
Is it correctness-one or performance?
The current hook feels a little bit weird.
Does it mean to completely ban speculative execution?
If then there is a lot more places where it should be used, and not using it there may be surprising.
Or does it only mean to ban PRE? But then, what was the original problem?
Or should it be per-MI hook?
etc

[anton-afanasyev]

In D66132#1626811, @lebedev.ri wrote:

I feel like this is lacking words.
What problem is this addressing?
Is it correctness-one or performance?
The current hook feels a little bit weird.
Does it mean to completely ban speculative execution?
If then there is a lot more places where it should be used, and not using it there may be surprising.
Or does it only mean to ban PRE? But then, what was the original problem?
Or should it be per-MI hook?
etc

The problem is discussed here: https://bugs.llvm.org/show_bug.cgi?id=42405
Per-MI hook already exists -- it is hasSideEffect(). But for particular targets setting this to true is undesirable, this breaks existing optimizations (though it could really have side effect, like div for aarch64. This change means to ban PRE at all, yes.
@igorb could add something here.

[lebedev.ri]

In D66132#1626871, @anton-afanasyev wrote:

In D66132#1626811, @lebedev.ri wrote:

I feel like this is lacking words.
What problem is this addressing?
Is it correctness-one or performance?
The current hook feels a little bit weird.
Does it mean to completely ban speculative execution?
If then there is a lot more places where it should be used, and not using it there may be surprising.
Or does it only mean to ban PRE? But then, what was the original problem?
Or should it be per-MI hook?
etc

The problem is discussed here: https://bugs.llvm.org/show_bug.cgi?id=42405
Per-MI hook already exists -- it is hasSideEffect(). But for particular targets setting this to true is undesirable, this breaks existing optimizations (though it could really have side effect, like div for aarch64.

This change means to ban PRE at all, yes.

So give it (the hook) less misleading name?

Should this be only a hook, or should there be some cl::opt (and how should it iteract with the hook?
Asking for completeness only, i guess just a hook may be fine.

@igorb could add something here.

[lebedev.ri]

After reading through https://bugs.llvm.org/show_bug.cgi?id=42405 i'm not confident that this is the correct solution.
It reads to me as-if this speculative execution in PRE being done with no proper legality checks.
In particular, i'd think you want to check a white-list in MachineCSE::isPRECandidate(),
much like llvm::isSafeToSpeculativelyExecute() is for middle-end.

Unless i'm really looking in the wrong place, there is no such thing here?

[anton-afanasyev]

In D66132#1626902, @lebedev.ri wrote:

After reading through https://bugs.llvm.org/show_bug.cgi?id=42405 i'm not confident that this is the correct solution.
It reads to me as-if this speculative execution in PRE being done with no proper legality checks.
In particular, i'd think you want to check a white-list in MachineCSE::isPRECandidate(),
much like llvm::isSafeToSpeculativelyExecute() is for middle-end.

Unless i'm really looking in the wrong place, there is no such thing here?

isPRECandidate() checks hasUnmodelledSideEffects() and mayRaiseFloatingPointException() serving as isSafeToSpeculativelyExecute().

[hfinkel]

TargetTransformInfo, in the title, should say TargetInstrInfo.

[lebedev.ri]

llvm/include/llvm/CodeGen/TargetInstrInfo.h

1026–1028

Regardless of my bikeshedding if this is the correct fix or not,
this should be renamed to be less misleading.
Just name it shouldDoPartialRedundancyElimination() ?

[anton-afanasyev]

Rename

[anton-afanasyev]

llvm/include/llvm/CodeGen/TargetInstrInfo.h

1026–1028

Ok, done

llvm/include/llvm/CodeGen/TargetInstrInfo.h

public:
 
   /// Return true when a target supports MachineCombiner.
   virtual bool useMachineCombiner() const { return false; }
 
   /// Return true if the given SDNode can be copied during scheduling
   /// even if it has glue.
   virtual bool canCopyGluedNodeDuringSchedule(SDNode *N) const { return false; }
 
+  /// Return true if speculative execution of target instructions is forbidden.
+  /// This prevents MIR code from SimplePRE optimization leading to possible
+  /// speculative execution of instructions.
+  virtual bool shouldDoPartialRedundancyElimination() const { return false; }
+
 protected:
   /// Target-dependent implementation for foldMemoryOperand.
   /// Target-independent code in foldMemoryOperand will
   /// take care of adding a MachineMemOperand to the newly created instruction.
   /// The instruction and any auxiliary instructions necessary will be inserted
   /// at InsertPt.
   virtual MachineInstr *
   foldMemoryOperandImpl(MachineFunction &MF, MachineInstr &MI,

llvm/lib/CodeGen/MachineCSE.cpp

bool MachineCSE::runOnMachineFunction(MachineFunction &MF) {
 
   TII = MF.getSubtarget().getInstrInfo();
   TRI = MF.getSubtarget().getRegisterInfo();
   MRI = &MF.getRegInfo();
   AA = &getAnalysis<AAResultsWrapperPass>().getAAResults();
   DT = &getAnalysis<MachineDominatorTree>();
   MBFI = &getAnalysis<MachineBlockFrequencyInfo>();
   LookAheadLimit = TII->getMachineCSELookAheadLimit();
-  bool ChangedPRE, ChangedCSE;
+  bool ChangedPRE = false, ChangedCSE;
+  if (!TII->shouldDoPartialRedundancyElimination()) {
     ChangedPRE = PerformSimplePRE(DT);
+  }
   ChangedCSE = PerformCSE(DT->getRootNode());
   return ChangedPRE || ChangedCSE;
 }