[llvm-commits] [llvm] r61980 - in /llvm/trunk: lib/Transforms/Scalar/InstructionCombining.cpp test/Transforms/InstCombine/bitcast-gep.ll

[Duncan Sands]

Hi Chris,

> >> +  // is something like [0 x {int, int}]
> >
> > if the type size is zero, I don't see how you can do anything unless
> > Offset is zero.  So how about handling the zero size case entirely
> > here and bailing out?
> 
> Consider trying to get to offset 4 in this.  It will form a gep 0, 0,  
> 1.  Just because the array has size zero it doesn't mean its elements  
> have size 0.

It seems rather weird that if you declare an object of type
[0 x {int, int}] then a GEP 0, 0, 1 will point outside the
object...

> >> +    // Handle silly modulus not returning values values [0..TySize).
...
> > Also, you would have to do something here anyway to correct FirstIdx,
> > but I see you don't complain about silly division :)
> 
> What do you mean?  Note that this code already existed in instcombine,  
> I just moved/refactored it.

OK, it's just that the comment seems misleading.  It suggests that
the problem is that modulus is being silly, but in fact there is a
real problem.  This is shown by the fact that it's not just the
modulus result that needs to be adjusted.

> >> +    assert((uint64_t)Offset < (uint64_t)TySize && "Out of range  
> >> offset");
> >
> > How about checking that FirstIdx * TySize + Offset == original offset?
> 
> why

One pointless assert deserves another :)

> Right, it doesn't (necessarily) remove the bitcast, this is a  
> canonicalization.  This happens a lot with union cases.

Yes, and a good one: turning bitcast of struct* into bitcast of scalar*
can only be a good thing :)

Ciao,

Duncan.