[PATCH] D116930: [DAGCombine] Fold SRA of a load into a narrower sign-extending load

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12142
+    // accessing any of the loaded bytes.
+    ShAmt = N1C->getZExtValue();
+    uint64_t MemoryWidth = LN0->getMemoryVT().getScalarSizeInBits();
----------------
spatel wrote:
> The various shift amount variables are difficult to follow. This is really a question for the existing code - can we make it clearer (either through renaming or code comments) how "ShAmt", "ShiftAmt" and "ShLeftAmt" are different?
> 
> If we can improve that, is it possible to make a small helper/lambda, so we can share the bailout conditions for SRA/SRL instead of duplicating code?
Yes. I thought the old names were kind of confusing. So I added the SRA separately. I'll make a separate patch, trying to improve the existing code as well.

One reason I did not share things with SRL (I started off trying to do it that way) is that the solution for SRL seemed to be divided into two parts. I believe the second part (that includes the hasOneUse guard) can be triggered also as being nestled inside an AND (N pointing at the AND and N0 being the SRL). Although, we can probably do some code sharing for the first part regardless of that, at least now when I understand that the unexplained "N0 = SDValue(N, 0)" only should be done only for SRL and not for SRA.



================
Comment at: llvm/test/CodeGen/X86/combine-sra-load.ll:74-75
 
 ; Negative test. All bits loaded from memory are shifted out, so we can fold
 ; away the shift.
 define i32 @sra_of_sextload_no_fold(i16* %p) {
----------------
spatel wrote:
> This comment is not accurate. We are replicating (splatting) the sign bit of the loaded i16 across 32-bits, so there's still a shift.
> In IR, instcombine would transform this into:
> 
> ```
> define i32 @sra_of_sextload_no_fold(i16* %p) {
>   %load = load i16, i16* %p, align 2
>   %1 = ashr i16 %load, 15
>   %shift = sext i16 %1 to i32
>   ret i32 %shift
> }
> 
> ```
Right, the comment is supposed to say "so we **can't** fold away the shift".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116930