non inline jumptables for arm

[muth02446, 2009-07-17 21:02:30]

http://codereview.appspot.com/96065/diff/1/2
File include/llvm/CodeGen/MachineConstantPool.h (right):

http://codereview.appspot.com/96065/diff/1/2#newcode51
Line 51: /// getJumpTableIndex - Check if this is a reference to a jump table.
this is confusing.
Indicate that this will likely be overloaded (maybe assert
inside to it) and move the comment to overloaded version

http://codereview.appspot.com/96065/diff/1/2#newcode54
Line 54: virtual unsigned *getJumpTableIndex() { return 0; }
why is it returning a pointer?
Should there be separate "Set" function

http://codereview.appspot.com/96065/diff/1/11
File lib/CodeGen/BranchFolding.cpp (right):

http://codereview.appspot.com/96065/diff/1/11#newcode249
Line 249: const std::vector<MachineConstantPoolEntry> &CPs =
this very magical 

could it be moved to a separate function with a meaningful name  like
"RemapJumpTableIndicesAndMarkTargetLive

http://codereview.appspot.com/96065/diff/1/7
File lib/Target/ARM/ARMISelLowering.cpp (right):

http://codereview.appspot.com/96065/diff/1/7#newcode1131
Line 1131: SDValue ARMTargetLowering::LowerJumpTable(SDValue Op, SelectionDAG
&DAG) {
this should be commented

http://codereview.appspot.com/96065/diff/1/7#newcode1134
Line 1134: MVT PTy = getPointerTy();
It would be nice to mark everything const that is readonly
but this does not seem to be common practice in this code base

http://codereview.appspot.com/96065/diff/1/7#newcode1141
Line 1141: Result = DAG.getNode(ARMISD::Wrapper, dl, PTy, Result);
I find chaining of variable in this way hard to read.
especially when this is done several time.
Result is also not a very meaningful name.

How about calling the first one ScaledIndex.

To somebody not deeply involved in the ARM code base the 
ARMISD::Wrapper node is mysterious and a comment would not hurt

http://codereview.appspot.com/96065/diff/1/6
File lib/Target/ARM/ARMInstrInfo.td (right):

http://codereview.appspot.com/96065/diff/1/6#newcode558
Line 558: // Indirect branches
probably only works for amv5 and higher (c.f. code using "blx" below)

http://codereview.appspot.com/96065/diff/12/13
File include/llvm/CodeGen/MachineConstantPool.h (right):

http://codereview.appspot.com/96065/diff/12/13#newcode51
Line 51: /// getJumpTableIndex - Check if this is a reference to a jump table.
This comment is confusing.
Maybe move it to the overloading function

http://codereview.appspot.com/96065/diff/12/13#newcode54
Line 54: virtual unsigned *getJumpTableIndex() { return 0; }
maybe always assert inside this function as it should have been overloaded when
called.

http://codereview.appspot.com/96065/diff/12/13#newcode56
Line 56: /// print - Implement operator<<
instead of returning a pointer add a "setter" function
and use getType to determine where it makes sense to call it

http://codereview.appspot.com/96065/diff/12/22
File lib/CodeGen/BranchFolding.cpp (right):

http://codereview.appspot.com/96065/diff/12/22#newcode248
Line 248: 
This is very magical block, could it be moved to a separate function with a
meaningful name like "RemapJumpTableIndicesAndMarkTargetLive

http://codereview.appspot.com/96065/diff/12/22#newcode255
Line 255: if (!JTIndex) continue;
use getType() test

http://codereview.appspot.com/96065/diff/12/22#newcode256
Line 256: *JTIndex = JTMapping[*JTIndex];
use setter

http://codereview.appspot.com/96065/diff/12/18
File lib/Target/ARM/ARMISelLowering.cpp (right):

http://codereview.appspot.com/96065/diff/12/18#newcode1138
Line 1138: 
this function should be better commented.

also, it would be nice to mark everything const that is readonly but this does
not seem to be common practice in this code base

http://codereview.appspot.com/96065/diff/12/18#newcode1149
Line 1149: Result = DAG.getNode(ARMISD::Wrapper, dl, PTy, Result);
I find chaining of variable in this way hard to read. especially when this is
done several time. Result is also not a very meaningful name. How about calling
the first one ScaledIndex. To somebody not deeply involved in the ARM code base
the ARMISD::Wrapper node is mysterious and a comment would not hurt

http://codereview.appspot.com/96065/diff/12/17
File lib/Target/ARM/ARMInstrInfo.td (right):

http://codereview.appspot.com/96065/diff/12/17#newcode590
Line 590: let isBranch = 1, isTerminator = 1, isBarrier = 1, isIndirectBranch =
1 in {
probably only works for amv5 and higher (c.f. code using "blx" below)