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

Tue Sep 11 16:47:00 PDT 2012

On Tue, 11 Sep 2012 15:14:29 +0000
"Villmow, Micah" <Micah.Villmow at amd.com> wrote:

> Ping

Micah,

This patch seems fine to me. Since switching my BG/Q target from using
v4i64 to using v4i1, my local regression tests no longer adequately
cover this patch. Nevertheless, as we've discussed, it is very similar
to a patch that I had developed, and applying it to my local codebase
does not indicate any problems. A fix to this issue is clearly needed,
and should be committed.

I am not sure who should officially okay this change (Evan?), but if no
one objects, I think you should commit.

 -Hal

> 
> > -----Original Message-----
> > From: Villmow, Micah
> > Sent: Wednesday, September 05, 2012 4:06 PM
> > To: Villmow, Micah; Hal Finkel
> > Cc: llvm-commits at cs.uiuc.edu
> > Subject: RE: [llvm-commits] [LLVMdev] RFC: CondCodeActions refactor
> > (was RE: Why is this assertion here?)
> > 
> > New patch attached, is this ok to commit?
> > 
> > Thanks,
> > Micah
> > 
> > > -----Original Message-----
> > > From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-
> > > bounces at cs.uiuc.edu] On Behalf Of Villmow, Micah
> > > Sent: Wednesday, August 15, 2012 10:08 AM
> > > To: Hal Finkel
> > > Cc: llvm-commits at cs.uiuc.edu
> > > Subject: Re: [llvm-commits] [LLVMdev] RFC: CondCodeActions
> > > refactor (was
> > > RE: Why is this assertion here?)
> > >
> > > Nothing really, looks like the diff program got confused.
> > >
> > > Micah
> > >
> > > > -----Original Message-----
> > > > From: Hal Finkel [mailto:hfinkel at anl.gov]
> > > > Sent: Wednesday, August 15, 2012 10:06 AM
> > > > To: Villmow, Micah
> > > > Cc: llvm-commits at cs.uiuc.edu
> > > > Subject: Re: [llvm-commits] [LLVMdev] RFC: CondCodeActions
> > > > refactor (was RE: Why is this assertion here?)
> > > >
> > > > Micah,
> > > >
> > > > Quick question:
> > > > > -           (unsigned)VT.getSimpleVT().SimpleTy <
> > > > > sizeof(CondCodeActions[0])*4 &&
> > > > > +           (unsigned)VT.getSimpleVT().SimpleTy <
> > > > > sizeof(CondCodeActions[0])*4 &&
> > > >
> > > > What's the change here? I don't see one.
> > > >
> > > >  -Hal
> > > >
> > > > On Tue, 14 Aug 2012 15:17:17 +0000
> > > > "Villmow, Micah" <Micah.Villmow at amd.com> wrote:
> > > >
> > > > > Sorry, wrong file.
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Villmow, Micah
> > > > > > Sent: Tuesday, August 14, 2012 8:17 AM
> > > > > > To: Villmow, Micah; Hal Finkel
> > > > > > Cc: llvm-commits at cs.uiuc.edu
> > > > > > Subject: RE: [llvm-commits] [LLVMdev] RFC: CondCodeActions
> > > > > > refactor (was RE: Why is this assertion here?)
> > > > > >
> > > > > > Ping, any comments here?
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: llvm-commits-bounces at cs.uiuc.edu
> > > > > > > [mailto:llvm-commits- bounces at cs.uiuc.edu] On Behalf Of
> > > > > > > Villmow, Micah Sent: Thursday, July 26, 2012 3:44 PM
> > > > > > > To: Hal Finkel
> > > > > > > Cc: llvm-commits at cs.uiuc.edu
> > > > > > > Subject: Re: [llvm-commits] [LLVMdev] RFC: CondCodeActions
> > > > > > > refactor (was
> > > > > > > RE: Why is this assertion here?)
> > > > > > >
> > > > > > > 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
> > > > >
> > > >
> > > >
> > > >
> > > > --
> > > > Hal Finkel
> > > > Postdoctoral Appointee
> > > > Leadership Computing Facility
> > > > Argonne National Laboratory
> > >
> > >
> > >
> > > _______________________________________________
> > > llvm-commits mailing list
> > > llvm-commits at cs.uiuc.edu
> > > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 

-- 
Hal Finkel
Postdoctoral Appointee
Leadership Computing Facility
Argonne National Laboratory