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

[Villmow, Micah]

New patch attached. I've fixed the space issues also.

Micah

> -----Original Message-----
> From: Hal Finkel [mailto:hfinkel at anl.gov]
> Sent: Thursday, July 26, 2012 3:14 PM
> To: Villmow, Micah
> Cc: llvm-commits at cs.uiuc.edu
> Subject: Re: [LLVMdev] RFC: CondCodeActions refactor (was RE: Why is
> this assertion here?)
> 
> Micah,
> 
> >      assert((unsigned)CC < array_lengthof(CondCodeActions) &&
> > -           (unsigned)VT.getSimpleVT().SimpleTy <
> > sizeof(CondCodeActions[0])*4 &&
> > +           (unsigned)VT.getSimpleVT().SimpleTy <
> > sizeof(CondCodeActions[0]) * 4 && "Table isn't big enough!");
> 
> You've added additional spaces around the *, I'd remove them again.
> 
> Also, you're patch seems to be missing where you actually change the
> definition of the CondCodeActions array.
> 
> Please send an updated patch; then we'll need input from a code owner.
> 
>  -Hal
> 
> On Thu, 26 Jul 2012 21:47:13 +0000
> "Villmow, Micah" <Micah.Villmow at amd.com> wrote:
> 
> > Yeah just the ordering are the real difference. Also, I  use shifts
> > and masks instead of conditionals and modules. My patch is attached.
> > For me either patch is fine, but what LLVM has now is broken.
> >
> > Either patch is fine, just need approval from someone to submit.
> >
> > Micah
> >
> > > -----Original Message-----
> > > From: Hal Finkel [mailto:hfinkel at anl.gov]
> > > Sent: Thursday, July 26, 2012 2:39 PM
> > > To: Villmow, Micah
> > > Cc: Developers Mailing List
> > > Subject: Re: [LLVMdev] RFC: CondCodeActions refactor (was RE: Why is
> > > this assertion here?)
> > >
> > > 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
> 
> 
> 
> --
> Hal Finkel
> Postdoctoral Appointee
> Leadership Computing Facility
> Argonne National Laboratory

-------------- next part --------------
A non-text attachment was scrubbed...
Name: support_ccodes_over_32_types.patch
Type: application/octet-stream
Size: 1879 bytes
Desc: support_ccodes_over_32_types.patch
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120726/612aec59/attachment.obj>