Page MenuHomePhabricator

abinavpp (Abinav Puthan Purayil)
User

Projects

User does not belong to any projects.

User Details

User Since
Feb 14 2021, 4:15 AM (32 w, 1 d)

Recent Activity

Wed, Sep 22

abinavpp added a comment to D109671: [AMDGPU] Add BFE pattern matches for constrained shifts..

Can we more generally just fold out and of 31 since the shifts ignore the high bits anyway?

Did you mean https://alive2.llvm.org/ce/z/wE_G9a ?

Wed, Sep 22, 4:29 AM · Restricted Project
abinavpp requested review of D110231: [AMDGPU] Add constrained shift pattern matches..
Wed, Sep 22, 4:27 AM · Restricted Project

Mon, Sep 13

abinavpp added a comment to D109671: [AMDGPU] Add BFE pattern matches for constrained shifts..

Can we more generally just fold out and of 31 since the shifts ignore the high bits anyway?

Mon, Sep 13, 11:54 PM · Restricted Project
abinavpp added a comment to D109671: [AMDGPU] Add BFE pattern matches for constrained shifts..

I think we need the constrained shift pattern to be of the form (srl (shl $src,
(and (sub 32, $width), 31)), (and (sub 32, $width), 31)), which, on $width = 0,
will evaluate to (srl (shl $src, 32), 32) like the original pattern.

Mon, Sep 13, 11:49 PM · Restricted Project
abinavpp added a comment to D109671: [AMDGPU] Add BFE pattern matches for constrained shifts..

This will give the wrong result when $width is zero.

I'm not seeing the problem with 0?

Mon, Sep 13, 9:40 PM · Restricted Project

Sun, Sep 12

abinavpp requested review of D109671: [AMDGPU] Add BFE pattern matches for constrained shifts..
Sun, Sep 12, 9:57 PM · Restricted Project

Wed, Sep 1

abinavpp committed rG0baace537994: [DAGCombine] Add node level checks for fp-contract and fp-ninf in… (authored by abinavpp).
[DAGCombine] Add node level checks for fp-contract and fp-ninf in…
Wed, Sep 1, 11:04 PM
abinavpp closed D107551: [DAGCombine] Add node level checks for fp-contract and fp-ninf in visitFMULForFMADistributiveCombine()..
Wed, Sep 1, 11:03 PM · Restricted Project

Aug 24 2021

abinavpp added a comment to D107552: [InstCombine] Combine lshr of add that intends to get the carry as llvm.uadd.with.overflow.

Ping.

Aug 24 2021, 12:56 AM · Restricted Project
abinavpp updated subscribers of D108363: [AMDGPU] Propagate !noalias and !alias.scope metadata in AMDGPULowerKernelArguments for noalias arguments..

Same purpose as D105721?

Aug 24 2021, 12:14 AM · Restricted Project

Aug 19 2021

abinavpp added inline comments to D108363: [AMDGPU] Propagate !noalias and !alias.scope metadata in AMDGPULowerKernelArguments for noalias arguments..
Aug 19 2021, 5:15 AM · Restricted Project
abinavpp added inline comments to D108361: [InlineFunction] Extend addAliasScopeMetadata() for non inliner requirements..
Aug 19 2021, 4:26 AM · Restricted Project
abinavpp added a comment to D108361: [InlineFunction] Extend addAliasScopeMetadata() for non inliner requirements..

We need not review this until we're good with https://reviews.llvm.org/D108363.

Aug 19 2021, 3:55 AM · Restricted Project
abinavpp added reviewers for D108363: [AMDGPU] Propagate !noalias and !alias.scope metadata in AMDGPULowerKernelArguments for noalias arguments.: arsenm, foad, rampitec.
Aug 19 2021, 3:53 AM · Restricted Project
abinavpp updated the summary of D108363: [AMDGPU] Propagate !noalias and !alias.scope metadata in AMDGPULowerKernelArguments for noalias arguments..
Aug 19 2021, 3:51 AM · Restricted Project
abinavpp updated the summary of D108361: [InlineFunction] Extend addAliasScopeMetadata() for non inliner requirements..
Aug 19 2021, 3:50 AM · Restricted Project
abinavpp requested review of D108363: [AMDGPU] Propagate !noalias and !alias.scope metadata in AMDGPULowerKernelArguments for noalias arguments..
Aug 19 2021, 3:48 AM · Restricted Project
abinavpp requested review of D108361: [InlineFunction] Extend addAliasScopeMetadata() for non inliner requirements..
Aug 19 2021, 3:38 AM · Restricted Project

Aug 18 2021

abinavpp added a comment to D107551: [DAGCombine] Add node level checks for fp-contract and fp-ninf in visitFMULForFMADistributiveCombine()..

I don't have a commit access. @arsenm, can you commit this on my behalf? Here are my details:
user.name=Abinav Puthan Purayil
user.email=abinav.puthanpurayil@amd.com

Aug 18 2021, 2:32 AM · Restricted Project
abinavpp updated the diff for D107551: [DAGCombine] Add node level checks for fp-contract and fp-ninf in visitFMULForFMADistributiveCombine()..

Rebased.

Aug 18 2021, 2:28 AM · Restricted Project

Aug 10 2021

abinavpp added inline comments to D107551: [DAGCombine] Add node level checks for fp-contract and fp-ninf in visitFMULForFMADistributiveCombine()..
Aug 10 2021, 8:58 PM · Restricted Project
abinavpp updated the diff for D107551: [DAGCombine] Add node level checks for fp-contract and fp-ninf in visitFMULForFMADistributiveCombine()..

Rebased; Addressed review comments.

Aug 10 2021, 8:58 PM · Restricted Project
abinavpp added inline comments to D107552: [InstCombine] Combine lshr of add that intends to get the carry as llvm.uadd.with.overflow.
Aug 10 2021, 8:42 PM · Restricted Project
abinavpp updated the diff for D107552: [InstCombine] Combine lshr of add that intends to get the carry as llvm.uadd.with.overflow.

Rebased; Addressed review comments.

Aug 10 2021, 8:41 PM · Restricted Project

Aug 9 2021

abinavpp updated the diff for D107552: [InstCombine] Combine lshr of add that intends to get the carry as llvm.uadd.with.overflow.

Fixed the case in which the add of the uaddo won't dominate the original trunc use. For e.g.:

Aug 9 2021, 2:36 AM · Restricted Project
abinavpp added a comment to D107552: [InstCombine] Combine lshr of add that intends to get the carry as llvm.uadd.with.overflow.

Most of non-correctness bailouts here don't really make sense to me.
Presumably, if it is profitable for a particular backend, said backend should be expanding whatever "bad" intrinsics into it's original form.

Aug 9 2021, 2:35 AM · Restricted Project
abinavpp added a comment to D107552: [InstCombine] Combine lshr of add that intends to get the carry as llvm.uadd.with.overflow.

It appears that we should at least be limiting it to legal types.

Aug 9 2021, 1:06 AM · Restricted Project
abinavpp updated the diff for D107552: [InstCombine] Combine lshr of add that intends to get the carry as llvm.uadd.with.overflow.

Rebased; Addressed review comments.

Aug 9 2021, 1:06 AM · Restricted Project

Aug 8 2021

abinavpp added inline comments to D107551: [DAGCombine] Add node level checks for fp-contract and fp-ninf in visitFMULForFMADistributiveCombine()..
Aug 8 2021, 9:46 PM · Restricted Project
abinavpp updated the diff for D107551: [DAGCombine] Add node level checks for fp-contract and fp-ninf in visitFMULForFMADistributiveCombine()..

Rebased; Addressed review comments.

Aug 8 2021, 9:46 PM · Restricted Project

Aug 5 2021

abinavpp added a comment to D107551: [DAGCombine] Add node level checks for fp-contract and fp-ninf in visitFMULForFMADistributiveCombine()..

This patch has no description summary at all :( What's the background to this? Is it related to PR51346 / D74436?

Aug 5 2021, 4:41 AM · Restricted Project
abinavpp added a comment to D106139: [DAGCombine] Combine srX of add that intends to get the carry as uaddo.

I pushed the inst-combine version of this at https://reviews.llvm.org/D107552.

Aug 5 2021, 4:29 AM · Restricted Project
abinavpp updated the summary of D107552: [InstCombine] Combine lshr of add that intends to get the carry as llvm.uadd.with.overflow.
Aug 5 2021, 3:56 AM · Restricted Project
abinavpp requested review of D107552: [InstCombine] Combine lshr of add that intends to get the carry as llvm.uadd.with.overflow.
Aug 5 2021, 3:53 AM · Restricted Project
abinavpp added reviewers for D107551: [DAGCombine] Add node level checks for fp-contract and fp-ninf in visitFMULForFMADistributiveCombine().: arsenm, nhaehnle, RKSimon, spatel.
Aug 5 2021, 3:49 AM · Restricted Project
abinavpp requested review of D107551: [DAGCombine] Add node level checks for fp-contract and fp-ninf in visitFMULForFMADistributiveCombine()..
Aug 5 2021, 3:45 AM · Restricted Project

Jul 28 2021

abinavpp added a comment to D106139: [DAGCombine] Combine srX of add that intends to get the carry as uaddo.

@lebedev.ri Can you confirm the revision you require in this?

Is there an issue with the code in this patch and/or do you want us to move this to inst-combine?
I still agree with @arsenm that this sort of thing might need to be done in both the places, in which case I don't see any issue in the order in which we push for it.

I agree that we might need to do this in both the places, emphasis is mine,
but mainly this should be done in middle-end, and afterwards the back-end fold
should be motivated by an end-to-end test showing the missed optimization.

Jul 28 2021, 6:46 AM · Restricted Project
abinavpp updated the diff for D106139: [DAGCombine] Combine srX of add that intends to get the carry as uaddo.

Rebased; Addressed review comments.

Jul 28 2021, 6:46 AM · Restricted Project

Jul 27 2021

abinavpp added a comment to D106139: [DAGCombine] Combine srX of add that intends to get the carry as uaddo.

@lebedev.ri Can you confirm the revision you require in this? Is there an issue with the code in this patch and/or do you want us to move this to inst-combine? I still agree with @arsenm that this sort of thing might need to be done in both the places, in which case I don't see any issue in the order in which we push for it.

Jul 27 2021, 9:26 PM · Restricted Project
abinavpp added inline comments to D106139: [DAGCombine] Combine srX of add that intends to get the carry as uaddo.
Jul 27 2021, 9:39 AM · Restricted Project
abinavpp updated the diff for D106139: [DAGCombine] Combine srX of add that intends to get the carry as uaddo.

Fixed the use replacement and added more comments there.

Jul 27 2021, 9:35 AM · Restricted Project
abinavpp added a comment to D106139: [DAGCombine] Combine srX of add that intends to get the carry as uaddo.

Two comments:

  1. the transform you want is https://alive2.llvm.org/ce/z/NFDE9C i.e. you need to start from lshr i64 %z, 32, match it's operands, and do *not* look at it's uses.

We're not looking at lshr's uses here, but that of the add in lshr(add()). I would like this to cover truncUse() in AMDGPU/combine-srx-add.ll (g() in X86/addcarry.ll). I'm not sure if we can do that without looking at the add's uses.

  1. This is really something that should go into instcombine

@foad mentioned the same. While I understand that target-specific combines should go to lib/target/<target> with setTargetDAGCombine(), I'm not able to determine if the target independent ones should go to target independent version of inst-combine, DAG-combine, or both. I understand that keeping at both has the advantage of better coverage since inst-combine might catch the pattern that DAG-combine might not be able to catch or vice-versa. I'm not able to see any motivation in *moving* this to inst-combine, but I do see a motivation in *copying* this to inst-combine.

This is a very straight-forward fold for a simple IR pattern. There is absolutely nothing target-specific there.
Unless said pattern can be produced by the backend lowering, it should be done in the middle-end, not here.

Jul 27 2021, 4:33 AM · Restricted Project
abinavpp added inline comments to D106139: [DAGCombine] Combine srX of add that intends to get the carry as uaddo.
Jul 27 2021, 4:29 AM · Restricted Project
abinavpp updated the diff for D106139: [DAGCombine] Combine srX of add that intends to get the carry as uaddo.

Rebased; Addressed review comments.

Jul 27 2021, 4:27 AM · Restricted Project

Jul 26 2021

abinavpp added a comment to D106139: [DAGCombine] Combine srX of add that intends to get the carry as uaddo.

Two comments:

  1. the transform you want is https://alive2.llvm.org/ce/z/NFDE9C i.e. you need to start from lshr i64 %z, 32, match it's operands, and do *not* look at it's uses.

We're not looking at lshr's uses here, but that of the add in lshr(add()). I would like this to cover truncUse() in AMDGPU/combine-srx-add.ll (g() in X86/addcarry.ll). I'm not sure if we can do that without looking at the add's uses.

Jul 26 2021, 5:55 AM · Restricted Project
abinavpp added a reviewer for D106139: [DAGCombine] Combine srX of add that intends to get the carry as uaddo: craig.topper.
Jul 26 2021, 3:53 AM · Restricted Project
abinavpp added a comment to D106139: [DAGCombine] Combine srX of add that intends to get the carry as uaddo.

I moved this to the target independent DAG-combiner.

Jul 26 2021, 3:45 AM · Restricted Project
abinavpp added a comment to D106139: [DAGCombine] Combine srX of add that intends to get the carry as uaddo.

I still think there is something wrong if you need to consider the truncated and non truncated case.

I haven't found an input where this transformation can go wrong, I'm not sure if we missed a corner case.

Jul 26 2021, 3:31 AM · Restricted Project
abinavpp retitled D106139: [DAGCombine] Combine srX of add that intends to get the carry as uaddo from [AMDGPU] Combine srl of add that intends to get the carry as uaddo to [DAGCombine] Combine srX of add that intends to get the carry as uaddo.
Jul 26 2021, 3:24 AM · Restricted Project
abinavpp added inline comments to D106139: [DAGCombine] Combine srX of add that intends to get the carry as uaddo.
Jul 26 2021, 3:23 AM · Restricted Project
abinavpp updated the diff for D106139: [DAGCombine] Combine srX of add that intends to get the carry as uaddo.
  • Made the transformation target independent.
  • Covered SRA case.
  • Added more tests.
  • Rebased.
Jul 26 2021, 3:20 AM · Restricted Project

Jul 22 2021

abinavpp retitled D106139: [DAGCombine] Combine srX of add that intends to get the carry as uaddo from [AMDGPU] Combine srl of add that intends to get the carry of the add as addcarry to [AMDGPU] Combine srl of add that intends to get the carry as uaddo.
Jul 22 2021, 10:22 AM · Restricted Project
abinavpp added inline comments to D106139: [DAGCombine] Combine srX of add that intends to get the carry as uaddo.
Jul 22 2021, 9:36 AM · Restricted Project
abinavpp updated the diff for D106139: [DAGCombine] Combine srX of add that intends to get the carry as uaddo.

Rebased; Added negative tests

Jul 22 2021, 9:13 AM · Restricted Project

Jul 21 2021

abinavpp added a comment to D106139: [DAGCombine] Combine srX of add that intends to get the carry as uaddo.

s/ISD::ADDCARRY/ISD::UADDO seems to generate the v_cndmask_b32_e64. I'm thinking of making this target independent in a different patch later since I'm not confident if this will benefit all targets in LLVM at the moment.

Jul 21 2021, 10:40 AM · Restricted Project
abinavpp added inline comments to D106139: [DAGCombine] Combine srX of add that intends to get the carry as uaddo.
Jul 21 2021, 10:31 AM · Restricted Project
abinavpp updated the diff for D106139: [DAGCombine] Combine srX of add that intends to get the carry as uaddo.
  • s/addcarry/uaddo
  • Addressed some review comments
Jul 21 2021, 10:18 AM · Restricted Project

Jul 20 2021

abinavpp added a comment to D106139: [DAGCombine] Combine srX of add that intends to get the carry as uaddo.
Before I address the comments on the code, test-case, coverage etc. we need to
be on the same page on where and how this should be done. I can see 2
approaches, but I'm unable to decide between the 2:
Jul 20 2021, 9:30 AM · Restricted Project
abinavpp added a comment to D106139: [DAGCombine] Combine srX of add that intends to get the carry as uaddo.

Even though this patch reduces the number of adds in the added combine-srl-add.ll test, it introduces a copy from vcc_hi. I.e. we get:
v_add_co_u32_e32 v0, vcc, v0, v1
v_mov_b32_e32 v1, vcc_hi
v_add_co_u32_e32 v0, vcc, vcc_lo, v2
v_addc_co_u32_e32 v1, vcc, v3, v1, vcc

The problem is that:
%7:vgpr_32, %8:sreg_64 = V_ADD_CO_U32_e64 %0:vgpr_32, %1:vgpr_32, 0, implicit $exec
%9:vreg_64 = V_ADD_U64_PSEUDO killed %13:vreg_64, killed %8:sreg_64, implicit-def dead $vcc, implicit-def dead $exec, implicit $exec

after finalize-isel becomes:
%7:vgpr_32, %8:sreg_64 = V_ADD_CO_U32_e64 %0:vgpr_32, %1:vgpr_32, 0, implicit $exec
%18:vgpr_32 = COPY %13.sub0:vreg_64
%19:sgpr_32 = COPY %8.sub0:sreg_64
%20:vgpr_32 = COPY %13.sub1:vreg_64
%21:sgpr_32 = COPY %8.sub1:sreg_64
%14:vgpr_32, %16:sreg_64_xexec = V_ADD_CO_U32_e64 %18:vgpr_32, %19:sgpr_32, 0, implicit $exec

%21:sgpr_32 = COPY %8.sub1:sreg_64 is created by adding SrcReg1Sub1 to the MachineInstr HiHalf in the expansion of V_ADD_U64_PSEUDO in SITargetLowering::EmitInstrWithCustomInserter(). This is responsible for the vcc_hi copy. How can we eliminate the copy correctly? I only found clumsy ways to do this :(

This isn't a correct expansion. You cannot interpret the carry out directly as the source. This needs a v_cndmask_b32 to get a vector source

Jul 20 2021, 9:19 AM · Restricted Project

Jul 16 2021

abinavpp added reviewers for D106139: [DAGCombine] Combine srX of add that intends to get the carry as uaddo: arsenm, cdevadas.
Jul 16 2021, 4:18 AM · Restricted Project
abinavpp added a comment to D106139: [DAGCombine] Combine srX of add that intends to get the carry as uaddo.

Even though this patch reduces the number of adds in the added combine-srl-add.ll test, it introduces a copy from vcc_hi. I.e. we get:
v_add_co_u32_e32 v0, vcc, v0, v1
v_mov_b32_e32 v1, vcc_hi
v_add_co_u32_e32 v0, vcc, vcc_lo, v2
v_addc_co_u32_e32 v1, vcc, v3, v1, vcc

Jul 16 2021, 4:08 AM · Restricted Project
abinavpp updated the summary of D106139: [DAGCombine] Combine srX of add that intends to get the carry as uaddo.
Jul 16 2021, 3:38 AM · Restricted Project
abinavpp updated the summary of D106139: [DAGCombine] Combine srX of add that intends to get the carry as uaddo.
Jul 16 2021, 3:37 AM · Restricted Project
abinavpp requested review of D106139: [DAGCombine] Combine srX of add that intends to get the carry as uaddo.
Jul 16 2021, 3:28 AM · Restricted Project