[PATCH] D145002: [DAGCombiner] Make `(zext (sgt X, -1))` -> `(srl (not X), N-1)` work if typeof(zext)!=typeof(X)
Noah Goldstein via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Apr 12 11:36:33 PDT 2023
goldstein.w.n added inline comments.
================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12706
+ // to do this if we are directly returning the result of the setcc which goes
+ // into i1/i8.
+ if (CC == ISD::SETGT && isAllOnesConstant(Ones) &&
----------------
pengfei wrote:
> craig.topper wrote:
> > goldstein.w.n wrote:
> > > pengfei wrote:
> > > > goldstein.w.n wrote:
> > > > > pengfei wrote:
> > > > > > goldstein.w.n wrote:
> > > > > > > RKSimon wrote:
> > > > > > > > goldstein.w.n wrote:
> > > > > > > > > craig.topper wrote:
> > > > > > > > > > This is a somewhat x86 specific statement. i8 isn't a legal type on ARM/AArch64/RISC-V so setcc never has i8 type. It will be i1, i32, or i64 on those targets.
> > > > > > > > > >
> > > > > > > > > > So what's special about i8 setcc on x86 that's different than the larger i32, i64 types on other targets?
> > > > > > > > > > This is a somewhat x86 specific statement. i8 isn't a legal type on ARM/AArch64/RISC-V so setcc never has i8 type. It will be i1, i32, or i64 on those targets.
> > > > > > > > > >
> > > > > > > > > > So what's special about i8 setcc on x86 that's different than the larger i32, i64 types on other targets?
> > > > > > > > >
> > > > > > > > > Its a hack for `x86`. simple code like `return x > -1` will get `zext` the `i1` to `i8`. In cases where we are just returning the flag, best not to do this transformation.
> > > > > > > > Ideally we still need a better way to do this that isn't so x86 specific
> > > > > > > > Ideally we still need a better way to do this that isn't so x86 specific
> > > > > > >
> > > > > > > Makes sense. I think the way to do this is check if the use of the `zext` is just a return.
> > > > > > > I'm having trouble implementing that however. Do you know how to check if an SDNode is a `ret`?
> > > > > > `N->isMachineOpcode() && getInstrInfo()->get(N->getMachineOpcode()).isReturn()`
> > > > > Almost but `llvm::TargetInstrInfo` is still incomplete in DAGCombiner. I think will probably just make a TLI hook and disable this for other targets.
> > > > Maybe `DAG.getSubtarget().getInstrInfo()`?
> > > > I don't think `TargetInstrInfo` is incomplete here. It's already used in `SelectionDAGBuilder`.
> > > > Maybe `DAG.getSubtarget().getInstrInfo()`?
> > > > I don't think `TargetInstrInfo` is incomplete here. It's already used in `SelectionDAGBuilder`.
> > >
> > > I tried that. This is what I used:
> > >
> > > ```
> > > auto IsUsedForRet = [&](SDNode *Sn) {
> > > unsigned MaxUses = 50;
> > > for (auto Use : Sn->uses()) {
> > > if (Use->isMachineOpcode() && DAG.getSubtarget()
> > > .getInstrInfo()
> > > ->get(Use->getMachineOpcode())
> > > .isReturn())
> > > return true;
> > >
> > > // Limit incase use list is huge.
> > > if (MaxUses-- == 0)
> > > return true;
> > > }
> > > return false;
> > > };
> > > ```
> > >
> > > I'll play around a bit more before doing TLI version.
> > What makes a use by a return special? Why wouldn't the same apply to a live out of a basic block?
> Did you add its header `#include "llvm/CodeGen/TargetInstrInfo.h"`?
> Ideally we still need a better way to do this that isn't so x86 specific
Moved the hack to TLI.shouldAvoidTransformToShift so its only x86 now.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D145002/new/
https://reviews.llvm.org/D145002
More information about the llvm-commits
mailing list