[PATCH] D131492: [RISCV] Implement isMaskAndCmp0FoldingBeneficial hook

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1147
+  // extension is supported and the mask is a power of two. However, we
+  // conservatively return false if the mask would bit in an ANDI instruction,
+  // on the basis that it's possible the fold wouldn't decrease the
----------------
reames wrote:
> This bit of logic feels like you're covering up something else.  Moving around the and instruction in IR shouldn't directly cause the bexti to be used if the andi form is legal and profitable.  Do we maybe have a missing pattern?
> 
> As a guess, maybe this is intersecting with D131482?
This hook enables copying of the AND not just moving. If we're just going to emit an ANDI, it doesn't really make sense to enable copying it.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1147
+  // extension is supported and the mask is a power of two. However, we
+  // conservatively return false if the mask would bit in an ANDI instruction,
+  // on the basis that it's possible the fold wouldn't decrease the
----------------
craig.topper wrote:
> reames wrote:
> > This bit of logic feels like you're covering up something else.  Moving around the and instruction in IR shouldn't directly cause the bexti to be used if the andi form is legal and profitable.  Do we maybe have a missing pattern?
> > 
> > As a guess, maybe this is intersecting with D131482?
> This hook enables copying of the AND not just moving. If we're just going to emit an ANDI, it doesn't really make sense to enable copying it.
'would bit' -> 'would fit'?


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

https://reviews.llvm.org/D131492