[PATCH] D105347: [AMDGPU][GlobalISel] Legalization of G_ROTL and G_ROTR

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:5973-5974
+  unsigned RevFsh = !IsLeft ? TargetOpcode::G_FSHL : TargetOpcode::G_FSHR;
+  if (bool IsFShLegal = LI.isLegalOrCustom({FShOpc, {DstTy, AmtTy}}) ||
+                        LI.isLegalOrCustom({RevFsh, {DstTy, AmtTy}})) {
+    // If a funnel shift in the other direction is supported, use it.
----------------
This doesn't do what you think it does :) It sets `IsFShLegal` to the whole expression `LI.isLegalOrCustom({FShOpc, {DstTy, AmtTy}}) || LI.isLegalOrCustom({RevFsh, {DstTy, AmtTy}})`


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:5976
+    // If a funnel shift in the other direction is supported, use it.
+    if (!IsFShLegal) {
+      if (isPowerOf2_32(EltSizeInBits)) {
----------------
... so `IsFShLegal` is always true here.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105347/new/

https://reviews.llvm.org/D105347