Page MenuHomePhabricator

prathamesh (Prathamesh)
User

Projects

User does not belong to any projects.

User Details

User Since
Mar 18 2020, 10:22 PM (35 w, 5 d)

Recent Activity

Sep 13 2020

prathamesh added inline comments to D87430: [ARM] Add heuristic to avoid lowering calls to blx for Thumb1 in ARMTargetLowering::LowerCall.
Sep 13 2020, 9:02 PM · Restricted Project
prathamesh updated the summary of D87594: Make TargetInstrInfo::foldMemoryOperand conditionally add memory operands.
Sep 13 2020, 8:52 PM · Restricted Project
prathamesh requested review of D87594: Make TargetInstrInfo::foldMemoryOperand conditionally add memory operands.
Sep 13 2020, 8:51 PM · Restricted Project

Sep 11 2020

prathamesh added inline comments to D87430: [ARM] Add heuristic to avoid lowering calls to blx for Thumb1 in ARMTargetLowering::LowerCall.
Sep 11 2020, 5:03 AM · Restricted Project

Sep 9 2020

prathamesh requested review of D87430: [ARM] Add heuristic to avoid lowering calls to blx for Thumb1 in ARMTargetLowering::LowerCall.
Sep 9 2020, 8:54 PM · Restricted Project

Sep 2 2020

prathamesh added a comment to D79785: [ARM] Register pressure with -mthumb forces register reload before each call.

I was wondering if either of these approaches look good ?
(a) Add a default param to TargetInstrInfo::foldMemoryOperandImpl, say bool &AddMemoryOperands, and the target specific hook sets it accordingly.
This will require modifying other target hooks to set AddMemoryOperands to true. Or alternatively return std::pair ?

Sep 2 2020, 8:37 AM · Restricted Project
prathamesh added a comment to rG85b4d286d7b1: [ARM] Register pressure with -mthumb forces register reload before each call.
Sep 2 2020, 8:36 AM
prathamesh added a comment to D79785: [ARM] Register pressure with -mthumb forces register reload before each call.

Hi,
It seems the issue comes from TargetInstrInfo::foldMemoryOperand, which adds the memory operands of LoadMI to newly created MI.
It calls foldMemoryOperandImpl:

Sep 2 2020, 5:25 AM · Restricted Project

Sep 1 2020

prathamesh added a comment to D79785: [ARM] Register pressure with -mthumb forces register reload before each call.
Sep 1 2020, 5:44 AM · Restricted Project
prathamesh added a comment to D79785: [ARM] Register pressure with -mthumb forces register reload before each call.

Sorry for the breakage, I will take a look.

Sep 1 2020, 2:43 AM · Restricted Project

Aug 29 2020

prathamesh added a comment to D79785: [ARM] Register pressure with -mthumb forces register reload before each call.

Yeah. I'm happy with this. Looking forward to seeing any follow ups.

LGTM, thanks!

Aug 29 2020, 12:10 PM · Restricted Project

Aug 28 2020

prathamesh added inline comments to D79785: [ARM] Register pressure with -mthumb forces register reload before each call.
Aug 28 2020, 7:52 AM · Restricted Project
prathamesh updated the diff for D79785: [ARM] Register pressure with -mthumb forces register reload before each call.

Added check in this version to return nullptr if CPI >= MCP->getConstants().size().
Does the patch look OK ? Tested with make check-llvm.
Thanks.

Aug 28 2020, 7:49 AM · Restricted Project

Aug 26 2020

prathamesh updated the diff for D79785: [ARM] Register pressure with -mthumb forces register reload before each call.

Hi,
Thanks for the catch! I tried your example, and it crashed llc with my patch. In the attached patch, I followed your suggestion to dyn_cast CPE.Val.ConstVal to Function * and that worked.

Aug 26 2020, 8:31 AM · Restricted Project

Aug 19 2020

prathamesh added a comment to D79785: [ARM] Register pressure with -mthumb forces register reload before each call.

My results seem to agree, that this improves things and doesn't break anything (they were only codesize benchmarks, but they all built OK).

Great, thanks for testing! Is this patch OK to commit ?

Aug 19 2020, 5:49 AM · Restricted Project

Aug 11 2020

prathamesh added a comment to D79785: [ARM] Register pressure with -mthumb forces register reload before each call.

My results seem to agree, that this improves things and doesn't break anything (they were only codesize benchmarks, but they all built OK).

Great, thanks for testing! Is this patch OK to commit ?

Aug 11 2020, 2:34 AM · Restricted Project

Aug 10 2020

prathamesh updated the diff for D79785: [ARM] Register pressure with -mthumb forces register reload before each call.

Thanks for the suggestions. I finally was able to build llvm natively on arm. Benchmarking SPEC2006 for code-size with -march=armv6 revealed minimal differences with previous patch. It works for the original test case from tinycrypt library, but causes a code-size regression of ~10% for another function from same library, resulting in overall worse code-size. Likewise disabling the transform, gives similar results.
Should we go instead with the approach suggested by John (reattaching in current patch) ?
Implement foldMemoryOperand hook to fold tLDRpci, tBLX pair to tBL.
This has one issue above if COPY insns come in between and foldMemoryOperand gets passed tLDRpci and COPY instead.
I am not sure how to handle that. However, this approach improves somewhat on the original behavior without causing regressions for any test-cases. Benchmarking showed minimal difference in code-size for SPEC2006, but improves the original test-case in tinycrypt and avoids the 10% code-size regression.

Aug 10 2020, 11:02 PM · Restricted Project

Jul 28 2020

prathamesh added a comment to D79785: [ARM] Register pressure with -mthumb forces register reload before each call.

Hello.

Do you have some codesize number for this, across some large-ish test suite? I would recommend the llvm-test-suite if you can get it compiling on Thumb1 - at least enough to get codesize number from it.

Hi, Sorry for late response. Unfortunately, we are having some infra issues for benchmarking code-size natively on SPEC, and am getting consistent build failures (without patch) while building llvm natively on arm. Do you have any suggestions on how to cross compile LLVM testsuite with -march=armv6-m enabled ? I did a cross compilation with SPEC, and code-size difference across all object files that got built was around 6.14%. I will report more descriptive results for SPEC after our infra issues get resolved. In the interim, I am also trying to test my patch for chromium.

Jul 28 2020, 5:47 AM · Restricted Project

Jul 15 2020

prathamesh added a comment to D79785: [ARM] Register pressure with -mthumb forces register reload before each call.

That sounds very fragile - I can easily imagine future, unrelated changes removing that COPY, or emitting one in the case which doesn't currently have one, which would completely change the effect of this patch.

I think the right way to do this is to look through the COPYs, find all the uses of the constant pool load, and do this transformation if and only if there are fewer than three uses of the load.

The alternative would be to not do the direct->indirect transformation in the first place for Thumb1-only targets, since they don't have enough registers to make it worthwhile very often. That would be trading off some generated code quality for simpler implementation.

Jul 15 2020, 2:29 AM · Restricted Project

Jul 6 2020

prathamesh updated the diff for D79785: [ARM] Register pressure with -mthumb forces register reload before each call.

Hi,
Sorry for late response. In the attached patch, I added a couple of more constraints if we're compiling for Thumb1:

  1. Number of args passed + caller's num of args < total number of available regs
  2. Each arg to callee, is either a "pass thru" arg, OR 8-bit imm OR a constant load. The intent is to allow only those args that need a single register for computation.
Jul 6 2020, 4:43 AM · Restricted Project

Jun 22 2020

prathamesh added a comment to D79785: [ARM] Register pressure with -mthumb forces register reload before each call.

only if the register holding function's address got spilled

I don't see any checks for this in the code. Why doesn't this trigger on every tLDRpci -> tBLXr pair, even if the register did not get spilled?

Jun 22 2020, 3:10 AM · Restricted Project

Jun 16 2020

prathamesh added a comment to D79785: [ARM] Register pressure with -mthumb forces register reload before each call.

I'd expect to see some extra checks to decide if reverting the indirect-call optimisation is profitable or not. If the loaded address is used more than three times, without being spilled, then I think it's better to leave the calls as indirect ones. If you don't think this is ever likely to be worthwhile for thumb 1, then it would be better to modify ARMTargetLowering::LowerCall to not do the transformation in the first place.

Jun 16 2020, 10:55 PM · Restricted Project

Jun 13 2020

prathamesh updated the diff for D79785: [ARM] Register pressure with -mthumb forces register reload before each call.

Hi,
Thanks for the review, and sorry I forgot to add the test earlier.
Does the attached version look OK ?
I verified that it works to remove indirect calls for the original test-case uECC_shared_secret from tinycrypt.
Tested with make check-llvm showing no unexpected failures.

Jun 13 2020, 6:26 AM · Restricted Project

Jun 8 2020

prathamesh updated the diff for D79785: [ARM] Register pressure with -mthumb forces register reload before each call.

Hi John,
Thanks for the review and I am sorry for late response, I was on a leave.
I have the updated the patch with suggested changes, do they look in right direction ?
Testing with make check-llvm shows no unexpected failures.

Jun 8 2020, 4:51 AM · Restricted Project

May 12 2020

prathamesh created D79785: [ARM] Register pressure with -mthumb forces register reload before each call.
May 12 2020, 7:29 AM · Restricted Project