llvm dev - Jun 2016 - Suggestion / Help regarding new calling convention

On Tue, Jun 21, 2016 at 12:31 AM, Matthias Braun <matze at braunis.de>
wrote:
> I just discussed this with vivek on IRC (and I think we agreed on this):
>
> Let me first state the motivation clearly to ease later discussions:
> As far as the motivation for this change goes: Changing the calling
> convention allows us to choose whether a register is saved by the callee or
> the caller. Usually it is best to have a mix of both as too many caller
> saved registers leads to unnecessary save/restores when the called function
> turns out to only touch a fraction of the registers (as is typically for
> smaller leaf-functions of the call graph). While too many callee saved
> registers may lead to unnecessary saves/restores of registers even though
> the calling function didn't have a live value in the register anyway.
With
> IPRA the first problem is mitigated since we propagate the actually
> clobbered set of registers up the callgraph instead of relying on
> conventions, so it is best to aim for more caller saved registers (though
> we should check for code size increases and store/restore code being
> potentially less good than the tuned sequences generated during
> FrameLowering).
>
> To the disucssion at hand:
> - Introducing a new calling convention at the IR level is the wrong
> approach: The calling convention is mostly a contract when calling and
> being called across translation unit boundaries. The details about how this
> contract is fulfilled are part of CodeGen IMO but do not need to be visible
> at the IR level.
> - The only thing we want to influence here is which registers are saved by
> the callee. Changing TargetFrameLowering::determineCalleeSaves() is a good
> place to achieve this without affecting unrelated things like parameter and
> return value handling which would be part of the calling convention.
>
Hello Matthias,

As per our discussion, the above trick will make sure that there is no
callee saved registers and also we have thought that
RegUsageInfoCalculator.cpp is having regmask that will make caller to save
restore registers if both callee and caller is using any common register
but this would require following change in RegUsageInfoCalculator.cpp :

if (!F->hasLocalLinkage() || F->hasAddressTaken()) {
      const uint32_t *CallPreservedMask         TRI->getCallPreservedMask(MF,
MF.getFunction()->getCallingConv());
      // Set callee saved register as preserved.
      for (unsigned i = 0; i < RegMaskSize; ++i)
        RegMask[i] = RegMask[i] | CallPreservedMask[i];
  }

because RegUsageInfoCalculator.cpp marks register as preserved if MF's CC
preserves it. But While optimizing for callee saved register we need to
skip above code so that register save/restore code is adder around call
site.

Apart from that my hunch is that IPO inlining of static function also
creates problem for this ( I have this feeling because some test case from
test-suite fails when using -O > 0 ). I am still working on this.

Please share your thoughts on this.

Sincerely,
- Vivek

> - We could experiment with dynamically changing the number of caller saved
> registers in the future. I could imagine heuristics like functions called
> from many places using some callee saved registers in order to avoid code
> size increases because of extra spills/restores at all the call sites. We
> can hardly create new calling conventions for these combinations of marking
> registers as callee/caller saved.
>
> - Matthias
>
> On Jun 20, 2016, at 7:39 AM, vivek pandya via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
> Dear Community,
>
> To improve current interprocedural register allocation (IPRA) , we have
> planned to set callee saved registers to none for local functions,
> currently I am doing it in following way:
>
> if (F->hasLocalLinkage()  && !F->hasAddressTaken()) {
>    DEBUG(dbgs() << "Function has LocalLinkage \n");
> F->setCallingConv(CallingConv::GHC);
>  }
>
> but we think threre should be clean and properway to do this perhaps like:
>
> if (F->hasLocalLinkage() && !F->hasAddressTaken()) {
>     DEBUG(dbgs() << "Function has LocalLinkage \n");
>     F->setCallingConv(CallingConv::NO_Callee_Saved);
>   }
>
> So I would like to know any better suggestions and if it is better to add
> a new CC for this purpose then what aspects should be considered while
> defining a new CC. Actually in this case the new CC does not really
> required to define how parameters should be passed or any special rule for
> return value etc , it just required to set callee saved registers to be
> none. So what are the minimal things required to define such a CC?
>
> Other alternative that I have thought was to add new attribute for
> function and use it like following in
> TargetFrameLowering::determineCalleeSaves()
>
>  // In Naked functions we aren't going to save any registers.
>   if (MF.getFunction()->hasFnAttribute(Attribute::Naked))
>     return;
>
> Any suggestions / thoughts are welcomed !
>
> Sincerely,
> Vivek
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160625/186655f6/attachment.html>

> On Jun 24, 2016, at 11:35 AM, vivek pandya <vivekvpandya at
gmail.com> wrote:
> 
> 
> 
> On Tue, Jun 21, 2016 at 12:31 AM, Matthias Braun <matze at braunis.de
<mailto:matze at braunis.de>> wrote:
> I just discussed this with vivek on IRC (and I think we agreed on this):
> 
> Let me first state the motivation clearly to ease later discussions:
> As far as the motivation for this change goes: Changing the calling
convention allows us to choose whether a register is saved by the callee or the
caller. Usually it is best to have a mix of both as too many caller saved
registers leads to unnecessary save/restores when the called function turns out
to only touch a fraction of the registers (as is typically for smaller
leaf-functions of the call graph). While too many callee saved registers may
lead to unnecessary saves/restores of registers even though the calling function
didn't have a live value in the register anyway. With IPRA the first problem
is mitigated since we propagate the actually clobbered set of registers up the
callgraph instead of relying on conventions, so it is best to aim for more
caller saved registers (though we should check for code size increases and
store/restore code being potentially less good than the tuned sequences
generated during FrameLowering).
> 
> To the disucssion at hand:
> - Introducing a new calling convention at the IR level is the wrong
approach: The calling convention is mostly a contract when calling and being
called across translation unit boundaries. The details about how this contract
is fulfilled are part of CodeGen IMO but do not need to be visible at the IR
level.
> - The only thing we want to influence here is which registers are saved by
the callee. Changing TargetFrameLowering::determineCalleeSaves() is a good place
to achieve this without affecting unrelated things like parameter and return
value handling which would be part of the calling convention.
> Hello Matthias,
> 
> As per our discussion, the above trick will make sure that there is no
callee saved registers and also we have thought that RegUsageInfoCalculator.cpp
is having regmask that will make caller to save restore registers if both callee
and caller is using any common register but this would require following change
in RegUsageInfoCalculator.cpp :
> 
> if (!F->hasLocalLinkage() || F->hasAddressTaken()) {
>       const uint32_t *CallPreservedMask >        
TRI->getCallPreservedMask(MF, MF.getFunction()->getCallingConv());
>       // Set callee saved register as preserved.
>       for (unsigned i = 0; i < RegMaskSize; ++i)
>         RegMask[i] = RegMask[i] | CallPreservedMask[i];
>   }
> 
> because RegUsageInfoCalculator.cpp marks register as preserved if MF's
CC preserves it. But While optimizing for callee saved register we need to skip
above code so that register save/restore code is adder around call site.
Indeed some adjustment there should improve your results. I would however
recommend to use something like this to determine which registers got saved:

const MachineFrameInfo &MFI = *MF.getFrameInfo();
assert(MFI.isCalleeSavedInfoValid());
for (const CalleeSavedInfo &Info : MFI.getCalleeSavedInfo()) {
   // Mark Info.getReg() and all its subregisters as preserved here!
}

- Matthias
> 
> Apart from that my hunch is that IPO inlining of static function also
creates problem for this ( I have this feeling because some test case from
test-suite fails when using -O > 0 ). I am still working on this.
> 
> Please share your thoughts on this.
> 
> Sincerely,
> - Vivek
>  
> - We could experiment with dynamically changing the number of caller saved
registers in the future. I could imagine heuristics like functions called from
many places using some callee saved registers in order to avoid code size
increases because of extra spills/restores at all the call sites. We can hardly
create new calling conventions for these combinations of marking registers as
callee/caller saved.
> 
> - Matthias
> 
>> On Jun 20, 2016, at 7:39 AM, vivek pandya via llvm-dev <llvm-dev at
lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
>> 
>> Dear Community,
>> 
>> To improve current interprocedural register allocation (IPRA) , we have
planned to set callee saved registers to none for local functions, currently I
am doing it in following way:
>> 
>> if (F->hasLocalLinkage()  && !F->hasAddressTaken()) {
>>    DEBUG(dbgs() << "Function has LocalLinkage \n");
>> F->setCallingConv(CallingConv::GHC); 
>>  }
>> 
>> but we think threre should be clean and properway to do this perhaps
like:
>> 
>> if (F->hasLocalLinkage() && !F->hasAddressTaken()) {
>>     DEBUG(dbgs() << "Function has LocalLinkage \n");
>>     F->setCallingConv(CallingConv::NO_Callee_Saved);
>>   }
>> 
>> So I would like to know any better suggestions and if it is better to
add a new CC for this purpose then what aspects should be considered while
defining a new CC. Actually in this case the new CC does not really required to
define how parameters should be passed or any special rule for return value etc
, it just required to set callee saved registers to be none. So what are the
minimal things required to define such a CC?
>> 
>> Other alternative that I have thought was to add new attribute for
function and use it like following in
TargetFrameLowering::determineCalleeSaves()
>> 
>>  // In Naked functions we aren't going to save any registers.
>>   if (MF.getFunction()->hasFnAttribute(Attribute::Naked))
>>     return;
>> 
>> Any suggestions / thoughts are welcomed !
>> 
>> Sincerely,
>> Vivek
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160624/33f551db/attachment.html>

On Sat, Jun 25, 2016 at 4:06 AM, Matthias Braun <matze at braunis.de>
wrote:
>
> On Jun 24, 2016, at 11:35 AM, vivek pandya <vivekvpandya at
gmail.com> wrote:
>
>
>
> On Tue, Jun 21, 2016 at 12:31 AM, Matthias Braun <matze at
braunis.de> wrote:
>
>> I just discussed this with vivek on IRC (and I think we agreed on
this):
>>
>> Let me first state the motivation clearly to ease later discussions:
>> As far as the motivation for this change goes: Changing the calling
>> convention allows us to choose whether a register is saved by the
callee or
>> the caller. Usually it is best to have a mix of both as too many caller
>> saved registers leads to unnecessary save/restores when the called
function
>> turns out to only touch a fraction of the registers (as is typically
for
>> smaller leaf-functions of the call graph). While too many callee saved
>> registers may lead to unnecessary saves/restores of registers even
though
>> the calling function didn't have a live value in the register
anyway. With
>> IPRA the first problem is mitigated since we propagate the actually
>> clobbered set of registers up the callgraph instead of relying on
>> conventions, so it is best to aim for more caller saved registers
(though
>> we should check for code size increases and store/restore code being
>> potentially less good than the tuned sequences generated during
>> FrameLowering).
>>
>> To the disucssion at hand:
>> - Introducing a new calling convention at the IR level is the wrong
>> approach: The calling convention is mostly a contract when calling and
>> being called across translation unit boundaries. The details about how
this
>> contract is fulfilled are part of CodeGen IMO but do not need to be
visible
>> at the IR level.
>> - The only thing we want to influence here is which registers are saved
>> by the callee. Changing TargetFrameLowering::determineCalleeSaves() is
a
>> good place to achieve this without affecting unrelated things like
>> parameter and return value handling which would be part of the calling
>> convention.
>>
> Hello Matthias,
>
> As per our discussion, the above trick will make sure that there is no
> callee saved registers and also we have thought that
> RegUsageInfoCalculator.cpp is having regmask that will make caller to save
> restore registers if both callee and caller is using any common register
> but this would require following change in RegUsageInfoCalculator.cpp :
>
> if (!F->hasLocalLinkage() || F->hasAddressTaken()) {
>       const uint32_t *CallPreservedMask >        
TRI->getCallPreservedMask(MF, MF.getFunction()->getCallingConv());
>       // Set callee saved register as preserved.
>       for (unsigned i = 0; i < RegMaskSize; ++i)
>         RegMask[i] = RegMask[i] | CallPreservedMask[i];
>   }
>
> because RegUsageInfoCalculator.cpp marks register as preserved if MF's
CC
> preserves it. But While optimizing for callee saved register we need to
> skip above code so that register save/restore code is adder around call
> site.
>
> Indeed some adjustment there should improve your results.
>
I think before improvement I need to get this functionally correct while
 local functions are optimized not to have callee saved registers, caller
is not saving appropriate registers as
TargetFrameLowering::determineCalleeSaves()
just effects callee saved register. RegMask needs to be updated to reflect
change too.
-Vivek
> I would however recommend to use something like this to determine which
> registers got saved:
>
> const MachineFrameInfo &MFI = *MF.getFrameInfo();
> assert(MFI.isCalleeSavedInfoValid());
> for (const CalleeSavedInfo &Info : MFI.getCalleeSavedInfo()) {
>    // Mark Info.getReg() and all its subregisters as preserved here!
> }
>
> - Matthias
>
>
> Apart from that my hunch is that IPO inlining of static function also
> creates problem for this ( I have this feeling because some test case from
> test-suite fails when using -O > 0 ). I am still working on this.
>
> Please share your thoughts on this.
>
> Sincerely,
> - Vivek
>
>
>> - We could experiment with dynamically changing the number of caller
>> saved registers in the future. I could imagine heuristics like
functions
>> called from many places using some callee saved registers in order to
avoid
>> code size increases because of extra spills/restores at all the call
sites.
>> We can hardly create new calling conventions for these combinations of
>> marking registers as callee/caller saved.
>>
>> - Matthias
>>
>> On Jun 20, 2016, at 7:39 AM, vivek pandya via llvm-dev <
>> llvm-dev at lists.llvm.org> wrote:
>>
>> Dear Community,
>>
>> To improve current interprocedural register allocation (IPRA) , we have
>> planned to set callee saved registers to none for local functions,
>> currently I am doing it in following way:
>>
>> if (F->hasLocalLinkage()  && !F->hasAddressTaken()) {
>>    DEBUG(dbgs() << "Function has LocalLinkage \n");
>> F->setCallingConv(CallingConv::GHC);
>>  }
>>
>> but we think threre should be clean and properway to do this perhaps
like:
>>
>> if (F->hasLocalLinkage() && !F->hasAddressTaken()) {
>>     DEBUG(dbgs() << "Function has LocalLinkage \n");
>>     F->setCallingConv(CallingConv::NO_Callee_Saved);
>>   }
>>
>> So I would like to know any better suggestions and if it is better to
add
>> a new CC for this purpose then what aspects should be considered while
>> defining a new CC. Actually in this case the new CC does not really
>> required to define how parameters should be passed or any special rule
for
>> return value etc , it just required to set callee saved registers to be
>> none. So what are the minimal things required to define such a CC?
>>
>> Other alternative that I have thought was to add new attribute for
>> function and use it like following in
>> TargetFrameLowering::determineCalleeSaves()
>>
>>  // In Naked functions we aren't going to save any registers.
>>   if (MF.getFunction()->hasFnAttribute(Attribute::Naked))
>>     return;
>>
>> Any suggestions / thoughts are welcomed !
>>
>> Sincerely,
>> Vivek
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160625/0d62268e/attachment-0001.html>