llvm dev - Sep 2011 - [LLVMdev] Possible bug in SimpleRegisterCoalescing

While working on a back-end for a target, I've come across something I
believe to be a bug in SimpleRegisterCoalescing.cpp. I'm unsure how /
whether to report it because I don't think it will necessarily crash or
generate incorrect code for any of the supported targets.

I believe that there may be a problem in
SimpleRegisterCoalescing::runOnMachineFunction where the allocatable registers
for each register class are initialised for the function, i.e. the lines:

  for (TargetRegisterInfo::regclass_iterator I = tri_->regclass_begin(),
         E = tri_->regclass_end(); I != E; ++I)
    allocatableRCRegs_.insert(std::make_pair(*I,
                                             tri_->getAllocatableSet(fn,
*I)));

If the allocatable registers are dependent on the function, such as might occur
when a frame pointer isn't required, then it seems that every function will
use the same allocatable set as the first function. [ a DenseMap insert
operation has no effect if the key is already present ]

The symptom that I'm seeing with my back-end can be fixed if I add:

    allocatableRCRegs_.erase(*I);

before the insert() operation.

I'd be grateful if anyone could confirm whether or not this is a genuine bug
and, if so, how best to report it.

My work has been based off LLVM 2.9.

Thanks

Steve Montgomery

On Sep 12, 2011, at 10:56 AM, Steve Montgomery wrote:
> My work has been based off LLVM 2.9.
The register allocator has been mostly rewritten. Try top of tree.

-eric
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20110912/fe90b9df/attachment.html>

On Sep 12, 2011, at 10:56 AM, Steve Montgomery wrote:
> While working on a back-end for a target, I've come across something I
believe to be a bug in SimpleRegisterCoalescing.cpp. I'm unsure how /
whether to report it because I don't think it will necessarily crash or
generate incorrect code for any of the supported targets.
> 
> I believe that there may be a problem in
SimpleRegisterCoalescing::runOnMachineFunction where the allocatable registers
for each register class are initialised for the function, i.e. the lines:
> 
>  for (TargetRegisterInfo::regclass_iterator I = tri_->regclass_begin(),
>         E = tri_->regclass_end(); I != E; ++I)
>    allocatableRCRegs_.insert(std::make_pair(*I,
>                                             tri_->getAllocatableSet(fn,
*I)));
> 
> If the allocatable registers are dependent on the function, such as might
occur when a frame pointer isn't required, then it seems that every function
will use the same allocatable set as the first function. [ a DenseMap insert
operation has no effect if the key is already present ]
As Eric said, this code is completely different on trunk.

Specifically, the RegisterClassInfo class now caches this information.  It does
cache information between functions, but it should invalidate the cache when the
set of reserved registers change:

  // Different reserved registers?
  BitVector RR = TRI->getReservedRegs(*MF);
  if (RR != Reserved)
    Update = true;
  Reserved = RR;

  // Invalidate cached information from previous function.
  if (Update)
    ++Tag;

Please verify that it works, though.  As you can see, the set of reserved
registers doesn't normally change.

/jakob

Thanks for your help. I've tried building from the trunk and it works, as
you'd suggested.

On 13 Sep 2011, at 01:00, Jakob Stoklund Olesen wrote:
> 
> On Sep 12, 2011, at 10:56 AM, Steve Montgomery wrote:
> 
>> While working on a back-end for a target, I've come across
something I believe to be a bug in SimpleRegisterCoalescing.cpp. I'm unsure
how / whether to report it because I don't think it will necessarily crash
or generate incorrect code for any of the supported targets.
>> 
>> I believe that there may be a problem in
SimpleRegisterCoalescing::runOnMachineFunction where the allocatable registers
for each register class are initialised for the function, i.e. the lines:
>> 
>> for (TargetRegisterInfo::regclass_iterator I =
tri_->regclass_begin(),
>>        E = tri_->regclass_end(); I != E; ++I)
>>   allocatableRCRegs_.insert(std::make_pair(*I,
>>                                           
tri_->getAllocatableSet(fn, *I)));
>> 
>> If the allocatable registers are dependent on the function, such as
might occur when a frame pointer isn't required, then it seems that every
function will use the same allocatable set as the first function. [ a DenseMap
insert operation has no effect if the key is already present ]
> 
> As Eric said, this code is completely different on trunk.
> 
> Specifically, the RegisterClassInfo class now caches this information.  It
does cache information between functions, but it should invalidate the cache
when the set of reserved registers change:
> 
>  // Different reserved registers?
>  BitVector RR = TRI->getReservedRegs(*MF);
>  if (RR != Reserved)
>    Update = true;
>  Reserved = RR;
> 
>  // Invalidate cached information from previous function.
>  if (Update)
>    ++Tag;
> 
> Please verify that it works, though.  As you can see, the set of reserved
registers doesn't normally change.
> 
> /jakob
>