[PATCH] D150670: [WebAssembly] Disable generation of fshl/fshr for rotates

pmatos updated this revision to Diff 526559. pmatos added a comment. Fix up some spacing issues. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150670/new/ https://reviews.llvm.org/D150670 Files: clang/test/CodeGen/WebAssembly/wasm-rotate.c

[PATCH] D150670: [WebAssembly] Disable generation of fshl/fshr for rotates

pmatos updated this revision to Diff 526556. pmatos added a comment. Implement optimization when demanded bits are known, skip otherwise for rotates. @nikic Things look much better now. Thanks for your help with the changes in InstCombine. What do you think? Repository: rG LLVM Github

[PATCH] D150670: [WebAssembly] Disable generation of fshl/fshr for rotates

nikic added inline comments. Comment at: llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp:927-933 +if (I->getOperand(0) != I->getOperand(1)) { + APInt DemandedMaskLHS(DemandedMask.lshr(ShiftAmt)); + APInt

[PATCH] D150670: [WebAssembly] Disable generation of fshl/fshr for rotates

pmatos added inline comments. Comment at: llvm/test/Transforms/InstCombine/fsh.ll:664 +; CHECK-NEXT:[[T1:%.*]] = and i32 [[A:%.*]], -65536 +; CHECK-NEXT:[[T2:%.*]] = call i32 @llvm.fshl.i32(i32 [[T1]], i32 [[T1]], i32 16) ; CHECK-NEXT:ret i32 [[T2]]

[PATCH] D150670: [WebAssembly] Disable generation of fshl/fshr for rotates

pmatos added inline comments. Comment at: llvm/test/Transforms/InstCombine/fsh.ll:664 +; CHECK-NEXT:[[T1:%.*]] = and i32 [[A:%.*]], -65536 +; CHECK-NEXT:[[T2:%.*]] = call i32 @llvm.fshl.i32(i32 [[T1]], i32 [[T1]], i32 16) ; CHECK-NEXT:ret i32 [[T2]]

[PATCH] D150670: [WebAssembly] Disable generation of fshl/fshr for rotates

nikic added inline comments. Comment at: llvm/test/Transforms/InstCombine/fsh.ll:664 +; CHECK-NEXT:[[T1:%.*]] = and i32 [[A:%.*]], -65536 +; CHECK-NEXT:[[T2:%.*]] = call i32 @llvm.fshl.i32(i32 [[T1]], i32 [[T1]], i32 16) ; CHECK-NEXT:ret i32 [[T2]]

[PATCH] D150670: [WebAssembly] Disable generation of fshl/fshr for rotates

pmatos updated this revision to Diff 526044. pmatos added a comment. Update the patch by removing target specific changes in CGBuiltin. Leave fshl/fshr unchanged for rotates. This actually fixes a todo in fshl/r test. @nikic What do you think of the current patch? Repository: rG LLVM

[PATCH] D150670: [WebAssembly] Disable generation of fshl/fshr for rotates

craig.topper added a comment. In D150670#4367736 , @pmatos wrote: > In D150670#4352094 , @craig.topper > wrote: > >>> Preventing the simplification means adding target specific code in >>> instcombine which

[PATCH] D150670: [WebAssembly] Disable generation of fshl/fshr for rotates

nikic added a comment. In D150670#4368241 , @pmatos wrote: > In D150670#4368238 , @pmatos wrote: > >> In D150670#4352163 , @nikic wrote: >> >>> 1. Say that we prefer

[PATCH] D150670: [WebAssembly] Disable generation of fshl/fshr for rotates

pmatos added a comment. In D150670#4368238 , @pmatos wrote: > In D150670#4352163 , @nikic wrote: > >> 1. Say that we prefer preserving rotates over "simplifying" funnel shifts >> (ending up with the rot2

[PATCH] D150670: [WebAssembly] Disable generation of fshl/fshr for rotates

pmatos added a comment. In D150670#4352163 , @nikic wrote: > 1. Say that we prefer preserving rotates over "simplifying" funnel shifts > (ending up with the rot2 pattern). Basically by skipping the optimization at >

[PATCH] D150670: [WebAssembly] Disable generation of fshl/fshr for rotates

pmatos added a comment. In D150670#4352094 , @craig.topper wrote: >> Preventing the simplification means adding target specific code in >> instcombine which seems even worse than adding it here given as @dschuff >> pointed out, there's precedent with

[PATCH] D150670: [WebAssembly] Disable generation of fshl/fshr for rotates

pmatos added a comment. @nikic Thank you for the thorough suggestions above. I will have to look at this closer next week and will work on an alternative solution. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150670/new/

[PATCH] D150670: [WebAssembly] Disable generation of fshl/fshr for rotates

nikic added a comment. In D150670#4352055 , @pmatos wrote: > In D150670#4351147 , @nikic wrote: > >> This doesn't looks like a wasm specific problem. You get essentially the >> same issue on any target that has

[PATCH] D150670: [WebAssembly] Disable generation of fshl/fshr for rotates

craig.topper added a comment. > Preventing the simplification means adding target specific code in > instcombine which seems even worse than adding it here given as @dschuff > pointed out, there's precedent with x86. How harmful is it to avoid breaking rotate patterns even if the target

[PATCH] D150670: [WebAssembly] Disable generation of fshl/fshr for rotates

pmatos added a comment. In D150670#4351147 , @nikic wrote: > Lowering to a rotate intrinsic only "solves" this for wasm and will at the > same time make these rotates completely opaque to optimization -- heck, it > looks like we don't even support

[PATCH] D150670: [WebAssembly] Disable generation of fshl/fshr for rotates

pmatos added a comment. In D150670#4351147 , @nikic wrote: > This doesn't looks like a wasm specific problem. You get essentially the same > issue on any target that has a rotate instruction but no funnel shift > instruction. Here are just a couple

[PATCH] D150670: [WebAssembly] Disable generation of fshl/fshr for rotates

nikic requested changes to this revision. nikic added a comment. This revision now requires changes to proceed. This doesn't looks like a wasm specific problem. You get essentially the same issue on any target that has a rotate instruction but no funnel shift instruction. Here are just a couple

[PATCH] D150670: [WebAssembly] Disable generation of fshl/fshr for rotates

pmatos added a comment. Ready for review. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150670/new/ https://reviews.llvm.org/D150670 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D150670: [WebAssembly] Disable generation of fshl/fshr for rotates

pmatos updated this revision to Diff 523149. pmatos added a comment. Generate rotates for 64bits as well. Add tests. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150670/new/ https://reviews.llvm.org/D150670 Files:

[PATCH] D150670: [WebAssembly] Disable generation of fshl/fshr for rotates

pmatos added a comment. In D150670#4347150 , @dschuff wrote: > FWIW X86 seems to do something similar elsewhere in this file > (https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CGBuiltin.cpp#L985-L986), > although it doesn't seem common

[PATCH] D150670: [WebAssembly] Disable generation of fshl/fshr for rotates

dschuff added a comment. FWIW X86 seems to do something similar elsewhere in this file (https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CGBuiltin.cpp#L985-L986), although it doesn't seem common otherwise. I think I'd be OK with this approach (and it does seem better than

[PATCH] D150670: [WebAssembly] Disable generation of fshl/fshr for rotates

pmatos added a reviewer: spatel. pmatos added a comment. Herald added a subscriber: StephenFan. Adding Sanjay as a reviewer since he implemented the clang emitRotate which we patched. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150670/new/

[PATCH] D150670: [WebAssembly] Disable generation of fshl/fshr for rotates

pmatos added a comment. Proposal to fix https://github.com/llvm/llvm-project/issues/62703 This not ready to land yet but it should open a discussion on if this is a good fix for this issue. If we want to go this route we need: support for 64bits, tests. A quick summary of why this is needed.

[PATCH] D150670: [WebAssembly] Disable generation of fshl/fshr for rotates

pmatos created this revision. pmatos added reviewers: dschuff, tlively. Herald added subscribers: asb, wingo, ecnelises, sunfish, hiraditya, jgravelle-google, sbc100. Herald added a project: All. pmatos requested review of this revision. Herald added subscribers: llvm-commits, cfe-commits,