llvm dev - Jul 2012 - [LLVMdev] RFC: CondCodeActions refactor (was RE: Why is this assertion here?)

I'm trying to understand why this assertion is here.
LegalizeAction
  getCondCodeAction(ISD::CondCode CC, EVT VT) const {
    assert((unsigned)CC < array_lengthof(CondCodeActions) &&
           (unsigned)VT.getSimpleVT().SimpleTy < sizeof(CondCodeActions[0])*4
&&
           "Table isn't big enough!");
    LegalizeAction Action = (LegalizeAction)
      ((CondCodeActions[CC] >> (2*VT.getSimpleVT().SimpleTy)) & 3);
    assert(Action != Promote && "Can't promote condition
code!");
    return Action;
  }

The first part of the assertion I can understand, but why is there an assertion
that there are only 32 types? in TOT LLVM if this code is called with
v8f32,v2f64
or v4f64, this assert is triggered.
Shouldn't the assert be:
(unsigned)VT.getSimpleVT().SimpleTy < MVT::MAX_ALLOWED_VALUETYPE &&
or
(unsigned)VT.getSimpleVT().SimpleTy < MVT::LAST_VECTOR_VALUETYPE &&
?

Thanks,
Micah


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

Well, I found out the reason why this assert is here, and this is problematic.

CondCodeActions only supports up to 32 different value types. Since we are past
32, what LLVM has is broken.

Currently the 4 different Legalize states are stored in successive bits and
packed into a uin64_t, see TargetLowering.h.
/// CondCodeActions - For each condition code (ISD::CondCode) keep a
  /// LegalizeAction that indicates how instruction selection should
  /// deal with the condition code.
  uint64_t CondCodeActions[ISD::SETCC_INVALID];

What I suggest is the following:
Change the definition of CondCodeAction to:
  uint64_t CondCodeActions[ISD::SETCC_INVALID][2];

setCondCodeAction then becomes:
void setCondCodeAction(ISD::CondCode CC, MVT VT,
                         LegalizeAction Action) {
    assert(VT < MVT::LAST_VALUETYPE &&
           (unsigned)CC < array_lengthof(CondCodeActions) &&
           "Table isn't big enough!");
    CondCodeActions[(unsigned)CC][VT.SimplyTy >> 5] &= ~(uint64_t(3UL)
<< (VT.SimpleTy - 32)*2);
    CondCodeActions[(unsigned)CC][VT.SimpleTy >> 5] |= (uint64_t)Action
<< (VT.SimpleTy - 32)*2;
  }

getCondCodeAction then becomes:
LegalizeAction
  getCondCodeAction(ISD::CondCode CC, EVT VT) const {
    assert((unsigned)CC < array_lengthof(CondCodeActions) &&
           (unsigned)VT.getSimpleVT().SimpleTy < MVT::LAST_VECTOR_VALUETYPE
&&
           "Table isn't big enough!");
    LegalizeAction Action = (LegalizeAction)
      ((CondCodeActions[CC][VT.getSimpleVT().SimpleTy >> 5] >>
(2*(VT.getSimpleVT().SimpleTy - 32))) & 3);
    assert(Action != Promote && "Can't promote condition
code!");
    return Action;
  }


The other options are to use a BitVector, or to have a different array for each
Legalized action. This approach however seems to use the minimum amount of
memory/instructions.


Ideas?
Micah

From: llvmdev-bounces at cs.uiuc.edu [mailto:llvmdev-bounces at cs.uiuc.edu] On
Behalf Of Villmow, Micah
Sent: Thursday, July 26, 2012 11:29 AM
To: Developers Mailing List
Subject: [LLVMdev] Why is this assertion here?

I'm trying to understand why this assertion is here.
LegalizeAction
  getCondCodeAction(ISD::CondCode CC, EVT VT) const {
    assert((unsigned)CC < array_lengthof(CondCodeActions) &&
           (unsigned)VT.getSimpleVT().SimpleTy < sizeof(CondCodeActions[0])*4
&&
           "Table isn't big enough!");
    LegalizeAction Action = (LegalizeAction)
      ((CondCodeActions[CC] >> (2*VT.getSimpleVT().SimpleTy)) & 3);
    assert(Action != Promote && "Can't promote condition
code!");
    return Action;
  }

The first part of the assertion I can understand, but why is there an assertion
that there are only 32 types? in TOT LLVM if this code is called with
v8f32,v2f64
or v4f64, this assert is triggered.
Shouldn't the assert be:
(unsigned)VT.getSimpleVT().SimpleTy < MVT::MAX_ALLOWED_VALUETYPE &&
or
(unsigned)VT.getSimpleVT().SimpleTy < MVT::LAST_VECTOR_VALUETYPE &&
?

Thanks,
Micah


-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120726/42e32429/attachment.html>

On Thu, 26 Jul 2012 21:15:35 +0000
"Villmow, Micah" <Micah.Villmow at amd.com> wrote:
> Well, I found out the reason why this assert is here, and this is
> problematic.
> 
> CondCodeActions only supports up to 32 different value types. Since
> we are past 32, what LLVM has is broken.
> 
> Currently the 4 different Legalize states are stored in successive
> bits and packed into a uin64_t, see TargetLowering.h. ///
> CondCodeActions - For each condition code (ISD::CondCode) keep a ///
> LegalizeAction that indicates how instruction selection should ///
> deal with the condition code. uint64_t
> CondCodeActions[ISD::SETCC_INVALID];
> 
> What I suggest is the following:
> Change the definition of CondCodeAction to:
>   uint64_t CondCodeActions[ISD::SETCC_INVALID][2];
> 
> setCondCodeAction then becomes:
> void setCondCodeAction(ISD::CondCode CC, MVT VT,
>                          LegalizeAction Action) {
>     assert(VT < MVT::LAST_VALUETYPE &&
>            (unsigned)CC < array_lengthof(CondCodeActions) &&
>            "Table isn't big enough!");
>     CondCodeActions[(unsigned)CC][VT.SimplyTy >> 5] &>
~(uint64_t(3UL)  << (VT.SimpleTy - 32)*2);
> CondCodeActions[(unsigned)CC][VT.SimpleTy >> 5] |= (uint64_t)Action
> << (VT.SimpleTy - 32)*2; }
> 
> getCondCodeAction then becomes:
> LegalizeAction
>   getCondCodeAction(ISD::CondCode CC, EVT VT) const {
>     assert((unsigned)CC < array_lengthof(CondCodeActions) &&
>            (unsigned)VT.getSimpleVT().SimpleTy <
> MVT::LAST_VECTOR_VALUETYPE && "Table isn't big
enough!");
>     LegalizeAction Action = (LegalizeAction)
>       ((CondCodeActions[CC][VT.getSimpleVT().SimpleTy >> 5] >>
> (2*(VT.getSimpleVT().SimpleTy - 32))) & 3); assert(Action != Promote
> && "Can't promote condition code!"); return Action;
>   }
> 
> 
> The other options are to use a BitVector, or to have a different
> array for each Legalized action. This approach however seems to use
> the minimum amount of memory/instructions.
> 
> 
> Ideas?
Your approach seems very similar to how I've fixed this problem
locally (I think that the only difference is the order of the arrays).
I've attached my version of the fix so that you can compare. I think
that, as a practical matter, this is the most economical approach.

 -Hal
> Micah
> 
> From: llvmdev-bounces at cs.uiuc.edu
> [mailto:llvmdev-bounces at cs.uiuc.edu] On Behalf Of Villmow, Micah
> Sent: Thursday, July 26, 2012 11:29 AM To: Developers Mailing List
> Subject: [LLVMdev] Why is this assertion here?
> 
> I'm trying to understand why this assertion is here.
> LegalizeAction
>   getCondCodeAction(ISD::CondCode CC, EVT VT) const {
>     assert((unsigned)CC < array_lengthof(CondCodeActions) &&
>            (unsigned)VT.getSimpleVT().SimpleTy <
> sizeof(CondCodeActions[0])*4 && "Table isn't big
enough!");
>     LegalizeAction Action = (LegalizeAction)
>       ((CondCodeActions[CC] >> (2*VT.getSimpleVT().SimpleTy)) &
3);
>     assert(Action != Promote && "Can't promote condition
code!");
>     return Action;
>   }
> 
> The first part of the assertion I can understand, but why is there an
> assertion that there are only 32 types? in TOT LLVM if this code is
> called with v8f32,v2f64 or v4f64, this assert is triggered.
> Shouldn't the assert be:
> (unsigned)VT.getSimpleVT().SimpleTy < MVT::MAX_ALLOWED_VALUETYPE
&&
> or
> (unsigned)VT.getSimpleVT().SimpleTy < MVT::LAST_VECTOR_VALUETYPE
&&
> ?
> 
> Thanks,
> Micah
> 
> 


-- 
Hal Finkel
Postdoctoral Appointee
Leadership Computing Facility
Argonne National Laboratory
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ccsize.patch
Type: text/x-patch
Size: 2275 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120726/59a88290/attachment.bin>