This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by anton-afanasyev on Aug 13 2019, 3:13 AM.

Details

Summary

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

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

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptAug 13 2019, 3:13 AM

Test coverage missing.

anton-afanasyev edited the summary of this revision. (Show Details)Aug 13 2019, 3:15 AM

Update comment

igorb accepted this revision.Aug 13 2019, 4:08 AM
igorb added a subscriber: igorb.

LGTM

This revision is now accepted and ready to land.Aug 13 2019, 4:08 AM

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

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.

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.

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?

lebedev.ri retitled this revision from [CodeGen] Add `isSpeculativeExecutionForbidden()` to `TargetTransformInfo` to [CodeGen] Add `isSpeculativeExecutionForbidden()` to `TargetTransformInfo` (PRR42405).Aug 13 2019, 5:32 AM
lebedev.ri edited the summary of this revision. (Show Details)

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().

TargetTransformInfo, in the title, should say TargetInstrInfo.

anton-afanasyev retitled this revision from [CodeGen] Add `isSpeculativeExecutionForbidden()` to `TargetTransformInfo` (PRR42405) to [CodeGen] Add `isSpeculativeExecutionForbidden()` to `TargetInstrInfo` (PRR42405).Aug 13 2019, 9:30 AM
lebedev.ri added inline comments.Aug 13 2019, 10:02 AM
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 marked 2 inline comments as done.

Rename

anton-afanasyev retitled this revision from [CodeGen] Add `isSpeculativeExecutionForbidden()` to `TargetInstrInfo` (PRR42405) to [CodeGen] Add `shouldDoPartialRedundancyElimination()` to `TargetInstrInfo` (PRR42405).Aug 14 2019, 1:26 AM
anton-afanasyev edited the summary of this revision. (Show Details)
anton-afanasyev added inline comments.
llvm/include/llvm/CodeGen/TargetInstrInfo.h
1026–1028

Ok, done

asbirlea resigned from this revision.Sep 15 2021, 12:24 PM