For particular subtargets setting shouldDoPartialRedundancyElimination() to true
prevents MIR code from SimplePRE optimization.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 36656 Build 36655: arc lint + arc unit
Event Timeline
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.
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.
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().
llvm/include/llvm/CodeGen/TargetInstrInfo.h | ||
---|---|---|
1026–1028 | Regardless of my bikeshedding if this is the correct fix or not, |
llvm/include/llvm/CodeGen/TargetInstrInfo.h | ||
---|---|---|
1026–1028 | Ok, done |
Regardless of my bikeshedding if this is the correct fix or not,
this should be renamed to be less misleading.
Just name it shouldDoPartialRedundancyElimination() ?